fix: drain all completed batches in classic PiecewiseMergeJoin - #24349
Open
viirya wants to merge 1 commit into
Open
fix: drain all completed batches in classic PiecewiseMergeJoin#24349viirya wants to merge 1 commit into
viirya 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
approved these changes
Aug 14, 2026
saadtajwar
left a comment
Contributor
There was a problem hiding this comment.
Not a maintainer but this makes sense and LGTM!
Member
Author
|
Thanks @saadtajwar |
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
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.
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.