[SLT] Fix SLT code generation broken by the introduction of compiler-produced regions#6348
Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. The materializations.clearRegions() between circuits is the core fix; without it region indices would leak across multiple DBSPCircuits emitted into the same file, which is exactly what was breaking SLT codegen after compiler-produced regions landed. Manual SLT re-run is the right validation here since SLT itself is the test suite for this codepath.
Two cosmetic notes inline.
| args.add("cargo"); | ||
| args.add("check"); | ||
| if (Utilities.inCI()) { | ||
| if (!Utilities.inCI()) { |
There was a problem hiding this comment.
Worth a one-line comment on why compileAndCheckRust caps jobs only outside CI while compileAndTest does the same — symmetric now (good), but the original asymmetric version was almost certainly a typo nobody noticed. A short // Cap parallelism locally; CI controls -j via env next to both branches would prevent the next person from re-flipping it.
| boolean skipped = this.toSkip > 0; | ||
| boolean skipped = false; | ||
| for (ISqlTestOperation operation: file.fileContents) { | ||
| skipped = this.toSkip > 0; |
There was a problem hiding this comment.
Minor: skipped is now reassigned on every iteration but only ever derived from this.toSkip, which is mutated inside the loop body further down. Could you confirm the new semantics match the old (i.e. the previous boolean skipped = this.toSkip > 0 initialised once before the loop)? If toSkip decrements as we consume operations, the new code will flip skipped to false mid-file, which is the intended behaviour — but a one-line comment to that effect would make it obvious.
…produced regions Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Spotted a stray file in the force-push: sql-to-dbsp-compiler/slt/out3147113901903481972.tmp (14 lines of stale cargo check warnings, with an embedded /home/mbudiu/git/feldera/... path).
Almost certainly committed by accident — please drop it. Worth adding slt/*.tmp (or whatever the pattern is) to .gitignore so it can't happen again.
The actual fix (materializations.clearRegions() between DBSPCircuits in RustFileWriter.write) is good — that matches the report from #6346/#6347 about region indices leaking across compilation units.
The LOCAL_CARGO_JOBS = 6 extraction is a nice cleanup. Note: there's a behavior-change buried in compileAndCheckRust: the condition flipped from Utilities.inCI() to !Utilities.inCI(). If it was right before, this inverts who gets the --jobs 6 cap (was: CI capped, now: local capped). If the original was the bug, please call that out in the commit message — it's easy to miss in review.
| @@ -0,0 +1,14 @@ | |||
| warning: unused variable: `pipeline_config` | |||
There was a problem hiding this comment.
Looks accidentally committed — stale cargo warning output with a local /home/mbudiu/ path baked in. Please drop and gitignore.
| args.add("cargo"); | ||
| args.add("check"); | ||
| if (Utilities.inCI()) { | ||
| if (!Utilities.inCI()) { |
There was a problem hiding this comment.
Behavior change here: the condition was if (Utilities.inCI()) and is now if (!Utilities.inCI()). Worth a sentence in the commit message — was the previous polarity the bug being fixed, or did this flip by accident along with the constant extraction?
Describe Manual Test Plan
I reran manually all the SLT tests which failed overnight.
(There are also two small cosmetic changes related to testing)