Finish #1909: join batch growth coverage, per-row nrows, observable grow-refusal warning - #2064
Merged
Merged
Conversation
…owth The non-differential join batch producer grows its scratch from the 64-row floor while filling a batch, and col_rel_prepare_resize copies exactly nrows rows and, for a timestamped scratch, nrows timestamps. The growth oracle cases used only untimestamped, unprojected fixtures, and the projected case runs at seven rows per batch, so projection had never met a reallocation and a timestamped scratch had never grown. Add two cases that start at the floor, grow inside one batch, and compare with the one-shot oracle: a projected output of 90 rows, and a timestamped left and output of 141 rows, which also checks the sink publishes zero timestamps. Both fail when grow_scratch stops publishing nrows before the reserve. Refs #1909
The non-differential join batch producer zeroed the scratch's nrows while filling and synced it to the written count only inside grow_scratch, before the reserve. col_rel_prepare_resize copies exactly nrows rows and timestamps, so the invariant was "remember to sync before any reallocation": a second reallocation site added to the fill loop would silently drop every row written so far, which neither ASan nor UBSan reports. Publish nrows after each written row instead, as the differential producer already does, and drop the sync from grow_scratch. A relocation anywhere in the loop now sees the live prefix without a sync point to remember. Removing the per-row store fails the seven growth oracle cases, including the projected and timestamped ones. Refs #1909
The WARN that join_batch's grow_scratch emits when the scratch cannot grow was unobservable from the producer tests: the binary did not initialise the logger until its last test, so no producer case could see the line. Removing the warning, removing its once-per-producer gate, or rotating its reason strings all survived the suite. Capture the JOIN section to a per-process file under TMPDIR through WL_LOG_FILE, restoring the caller's WL_LOG and WL_LOG_FILE afterwards, and publish batch by batch. One case refuses the scratch resize of two consecutive batches and expects a single warning naming "allocation failed". The other leaves the governor room for a 64-row payload but not for growing to 128 rows, and expects "admission denied". Both then lift the refusal, finish the scan and compare with the one-shot oracle. Removing the warning, removing its gate, or rotating the reasons now fails. The case reports SKIP when the compile-time ceiling strips WARN. With this, #1909's remaining items are done. The differential producer's lazy, refusal-aware growth with per-row nrows, and its timestamped and projected growth case, came with c946139. The non-differential producer's growth coverage and per-row nrows came in the two preceding commits. Fixes #1909
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This closes #1909. Earlier work had already done most of it on the differential side: c946139 gave
diff_join_batch.clazy scratch growth that tells a refusal from an allocation failure, per-rownrows, and a timestamped and projected growth test. These three commits finish the remaining items on the non-differential producer,join_batch.c:nrowsis now kept current after each written row, as the differential producer already does, instead of being synced only insidegrow_scratchbefore the reserve. A future reallocation site has no sync point to forget. Removing the per-row store fails 7 growth oracle cases. Also corrects thecol_join_write_pair_atcomment, which now covers all four callers.WL_LOG/WL_LOG_FILE. It asserts a single warning that names "allocation failed" across two refused grows, and "admission denied" for a governor refusal, then finishes the scan against the oracle. It reports SKIP when the compile-time ceiling strips WARN.Validation
nrowssync or the per-row store fails 7 cases.meson test: 416 OK, 0 failed, 14 skipped.--suite tidy: 4/4 OK. uncrustify is clean.Fixes #1909