fix: drain all completed batches in classic PiecewiseMergeJoin - #24349
fix: drain all completed batches in classic PiecewiseMergeJoin#24349viirya wants to merge 1 commit into
Conversation
`ClassicPWMJStream::process_stream_batch` returned a single completed batch and then advanced to `FetchStreamBatch`. When a stream batch produced more than one `batch_size` output chunk, `finish_buffered_batch` left additional completed batches queued; advancing while they remained stranded them for the final stream batch, dropping output rows. This only manifested at small `batch_size` values (the default 8192 hides it). Only advance to `FetchStreamBatch` once all completed batches are drained (the head-of-function drain returns one per poll). Once the scan is done and the output is drained, advance without re-scanning. Closes apache#24348. Co-authored-by: Claude Code
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24349 +/- ##
==========================================
- Coverage 81.17% 81.17% -0.01%
==========================================
Files 1109 1109
Lines 388038 388049 +11
Branches 388038 388049 +11
==========================================
+ Hits 314992 314998 +6
- Misses 54507 54508 +1
- Partials 18539 18543 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
saadtajwar
left a comment
There was a problem hiding this comment.
Not a maintainer but this makes sense and LGTM!
|
Thanks @saadtajwar |
|
Thanks @buraksenn — I checked both and found that this PR is a duplicate. Same root cause (transitioning before the output coalescer is drained), but yours fixes it at both phase boundaries via Please go ahead with #24298 — no need to rebase onto it. I'll close #24349 and issue #24348 as duplicates of #24297. |
|
Closing as a duplicate of #24298 (which is more complete — see my comment above). |
Which issue does this PR close?
Rationale for this change
A classic
PiecewiseMergeJoincan drop output rows when a single stream batch produces more than onebatch_sizeoutput chunk.ClassicPWMJStream::process_stream_batchreturns one completed batch from the coalescer and then advances to fetch the next stream batch. Butfinish_buffered_batchcan leave several completed batches queued; advancing while some remain strands them for the final stream batch, so those rows are never emitted. It only shows up at smallbatch_size(the default 8192 usually keeps a stream batch's output within a single chunk).What changes are included in this PR?
process_stream_batch, only advance toFetchStreamBatchonce all completed batches are drained (the head-of-function drain returns one per poll). Once the scan is finished and the output is fully drained, advance without re-scanning (re-scanning would re-emit rows).Are these changes tested?
Yes.
pwmj.slt: aRIGHT JOINwithSET batch_size = 4over a 30-row dataset, assertingcount(*), count(l.id)=198 186. The test fails without this change (196) and passes with it.NestedLoopJoin(enable_piecewise_merge_joinon vs off): all join types, operators</<=/>/>=,batch_sizein {8,16,32,64,100,8192} and 1/4 partitions, random data with NULLs — 0 mismatches.Are there any user-facing changes?
PiecewiseMergeJoin(behindenable_piecewise_merge_join, default off) no longer drops rows at smallbatch_size, matchingNestedLoopJoin. No API changes.