Skip to content

perf(pwmj): build output batches from buffered runs and one streamed … - #26287

Merged
jayzhan211 merged 3 commits into
apache:mainfrom
SubhamSinghal:pwmj-classic-batched-emission
Nov 2, 2026
Merged

jayzhan211 merged 3 commits into
apache:mainfrom
SubhamSinghal:pwmj-classic-batched-emission

Conversation

@SubhamSinghal

@SubhamSinghal SubhamSinghal commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

On large inputs the classic PiecewiseMergeJoin (Inner/Left/Right/Full) can be slower than NestedLoopJoin. For example, before #25840, l JOIN r ON l.a < r.b over 100K×100K rows at the default target_partitions took 1.41s with PWMJ and 1.26s with NLJ.

A profile of the 1K×1M case (sample, target_partitions = 1) shows the time goes to copying output rather than to comparisons:

where share of busy samples
take repeating each matched streamed row 32%
BatchCoalescer memmove of every column 30%
downstream sum (the query itself) 10%
per-streamed-row fixed cost (RecordBatch, allocations, coalescer bookkeeping) ~13%

For every matched streamed row, the join takes UInt32Array::from_value(row, count), gathers the streamed row count times, slices the buffered suffix, and builds a RecordBatch from them. BatchCoalescer then copies all of it again, so the streamed columns are written twice. #25840 made finding each row's first match a binary search and settles never-matching rows before the sort, but it kept this emission path, so on current main the cost above is still paid for every matched streamed row.

What changes are included in this PR?

The classic join builds each output batch directly, and BatchCoalescer is removed from this path.

Matched rows:

  • The output is kept as runs: (buffered start, len), paired with one streamed row index per output row.
  • Once batch_size rows are pending, or the stream batch ends:
    • Buffered columns are copied run by run with MutableArrayData::try_extend, which is a memcpy for fixed-width types.
    • Streamed columns are gathered with one take per column.

Rows that match nothing are emitted with null columns for the other side, in batches of at most batch_size:

  • Streamed rows that can never match (Right/Full), which perf(pwmj):settle never-matching streamed rows against the buffered extreme… #25840 settles before the sort, are selected with one filter_record_batch and emitted as slices of its output (push_unmatched_streamed). When every row of a stream batch is unmatched, filter returns the input columns as a slice and copies nothing.
  • Unmatched buffered rows (Left/Full), in the final pass, are the prefix before the min_marked watermark plus any trailing NULL keys (fix: align nested piecewise merge join sorting and comparison #26043). They are copied out of the buffered batch (push_unmatched_buffered), not sliced: a slice would keep the whole buffered batch alive and report its full buffers to downstream memory accounting once per output batch

Benchmarks

The pwmj suite from #25839 (100K buffered keys, 2M streamed rows, five match regimes × INNER/LEFT/RIGHT/FULL). Reproduce with:

cargo run -p datafusion-benchmarks --release --bin benchmark_runner -- pwmj

regime main (ms) this PR (ms) speedup
no_match, null_heavy: Inner, Left 2.2–2.3 2.1 1.05×
no_match, null_heavy: Right, Full 3.2–3.3 2.4–2.5 1.29–1.33×
half_match 231–233 27–29 8.1–8.4×
selective 453–455 47–48 9.5–9.7×
all_match (200M output rows) 167–168 159–161 1.05–1.06×

What is the testing strategy for this PR?

Six new unit tests in classic_join.rs:

  • join_full_less_than_splits_suffixes_across_batches: at batch_size = 4, a buffered suffix straddles an output batch, and never-matching streamed rows, matched rows and unmatched buffered rows all appear. It asserts both the rows and the batch sizes [1, 4, 2, 1].
  • join_inner_less_than_one_row_completes_several_batches: one streamed row whose buffered suffix fills several output batches.
  • join_right_less_than_empty_buffered: a Right join with an empty buffered side, where every streamed row is padded with nulls.
  • join_right_less_than_never_matching_rows_split_by_batch_size: more never-matching streamed rows than batch_size, which must be split into batches of at most batch_size, in input order, ahead of the matched rows.
  • join_left_less_than_unmatched_prefix_and_trailing_nulls: a Left join on < whose unmatched buffered rows are a prefix longer than batch_size plus trailing NULL keys, which sort last for <.
  • join_left_unmatched_buffered_batches_report_own_memory: each unmatched-buffered output batch reports only its own rows' memory, not the whole buffered batch.

Are there any user-facing changes?

No API changes. EXPLAIN ANALYZE reports a smaller output_bytes for PiecewiseMergeJoin, because output batches no longer share the coalescer's buffers (explain_analyze.slt: 128.0 KB → 96.0 B).

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Oct 14, 2026
@codecov-commenter

codecov-commenter commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.15789% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.80%. Comparing base (0d79252) to head (76851c6).
⚠️ Report is 215 commits behind head on main.

Files with missing lines Patch % Lines
...lan/src/joins/piecewise_merge_join/classic_join.rs 88.12% 5 Missing and 40 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #26287    +/-   ##
========================================
  Coverage   82.80%   82.80%            
========================================
  Files        1149     1149            
  Lines      454146   454442   +296     
  Branches   454146   454442   +296     
========================================
+ Hits       376035   376288   +253     
- Misses      55260    55269     +9     
- Partials    22851    22885    +34     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SubhamSinghal

Copy link
Copy Markdown
Contributor Author

@comphead @jayzhan211 @kumarUjjawal Can you help in reviewing this PR. Thanks.

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SubhamSinghal , LGTM!

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the change statically and did not run it. I found no correctness problem in the min_marked handling, the resume across early returns or the final flush. The inline comments cover output batch sizes, string view payloads and one redundant parameter.

Tests

  • The new tests assert row content that piecewise_merge_join_matrix.slt (Inner/Left/Right/Full at batch sizes 1, 2, 100, 8192 against NestedLoopJoin) and fuzz_pwmj_matches_nested_loop (batch size 3, i32 and f64, several partitions) already compare row for row. Only the batch layout assertions are new coverage. join_right_less_than_empty_buffered has no layout assertion, so a RIGHT JOIN against an empty table in the matrix file would replace it.
  • join_full_less_than_splits_suffixes_across_batches and join_inner_less_than_one_row_completes_several_batches both split a buffered suffix across batch_size in push_matched. One of them is probably enough.
  • Every payload column in the new tests, the fuzz harness and the pwmj benchmark is Int32. copy_runs goes through MutableArrayData for every type, so a Utf8 and a Utf8View payload case would cover the paths that differ from fixed width.

Benchmarks

  • The pwmj suite has an Int32 payload and a single aggregate on top. It shows neither the string payload memory effect nor the batch size effect described inline.

Nits

  • buffered_runs: vec![] holds at most batch_size entries, so it can be sized once like streamed_indices.
  • The OutputBuffer doc narrates the old per-row design. The comparison fits the PR description. The code comment only needs the invariant: runs pair with streamed indices and each column is copied once per batch.
  • process_unmatched_buffered_batch calls push_unmatched_buffered twice and each call can end with a partial chunk. One copy_runs over both ranges would avoid the extra tail batch.

// can't wait for the next one
batch_process_state
.output_batches
.flush(buffered_batch, &stream_batch.batch)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flush ends every stream batch with a partial output batch. push_unmatched_streamed and push_unmatched_buffered also emit their own partial tails. BatchCoalescer used to merge these across stream batches, and HashJoinExec and NestedLoopJoinExec still do. With batch_size-row stream batches and a selective predicate, the join now emits roughly one batch of the matched rows per stream batch instead of full batch_size batches. The new tests show the layouts: [1, 4, 2, 1], [2, 2, 2, 1] and [1, 2, 1, 2].

The pwmj suite cannot show the cost because the query is one aggregate on top. Could you add numbers for an operator that pays per batch above the join (a GROUP BY or a RepartitionExec) on selective and half_match? If the cost is real, the pending rows would have to survive across stream batches.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comphead I checked this against main before merging. Main also ends every stream batch with a partial batch (next_drained_batch calls finish_buffered_batch before the next stream batch). With 200 stream batches that each match one row, both emit 200 output batches for Inner and 400 for Right. So the layout is unchanged by this PR. If you think merging tails across stream batches is worth doing, I can open a follow-up issue.

///
/// The rows are copied, not sliced: a slice would keep all of `buffered_batch` alive
/// and report its full buffers to memory accounting downstream, once per batch.
fn push_unmatched_buffered(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc says copying lets each batch report only its own memory. That holds for fixed width and Utf8/Binary. For Utf8View/BinaryView, MutableArrayData appends every data buffer of the source to the output (variadic_data_buffers in arrow-data transform/mod.rs), so each output batch still references the whole buffered batch. take on the streamed side does the same. BatchCoalescer compacts sparse view sources (need_gc in arrow-select coalesce/byte_view.rs), so the old path did not retain them.

Parquet strings are views by default, so downstream reservations (SortExec, RepartitionExec) could see up to the full buffered string data per output batch. This is from reading the arrow sources and is not measured. The memory test only uses Int32. Could you run it with a Utf8View column against main? If it regresses, view columns would need a compaction step.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comphead I measured this against main before merging. The setup is a 20K-row buffered side with 42-byte string payloads and 200 output batches, and the numbers are the sum of get_record_batch_memory_size over the output batches:

payload, case main this PR
Utf8View, each match 1000 rows 217.8 MB 217.7 MB
Utf8View, each match 100 rows 419 MB 237 MB
Utf8View, unmatched buffered rows 207 MB 207 MB
Utf8, each match 1000 rows 236 MB 20 MB
Utf8, each match 100 rows 20 MB 25 MB

Main doesn't compact the views at this scale either, so this isn't a regression. Compacting view columns in the join output would help both. If you'd like that tracked, I can open a follow-up issue.

// Streamed schema
streamed_schema: SchemaRef,
// Buffered schema
buffered_schema: SchemaRef,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buffered batch is built with concat_batches(&buffered.schema(), ..) in exec.rs, so buffered_data.batch().schema_ref() already gives this schema inside fetch_stream_batch. The new field, the constructor parameter and the exec.rs change can be dropped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in b0fcb65

.columns()
.iter()
.map(|column| {
let data = column.to_data();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_data() and MutableArrayData::try_new run for every buffered column on every flush, but the buffered batch does not change during the join. Holding the ArrayData per column once would leave only try_new per flush. It matters most for the small end-of-stream-batch flushes. Not measured.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in b0fcb65

@alamb

alamb commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

This PR appears to be waiting on the author to respond to reviewer feedback so marking as draft. Please mark ready to review when it is ready for another look

@alamb
alamb marked this pull request as draft October 24, 2026 10:04
@github-actions github-actions Bot added the core Core DataFusion crate label Oct 28, 2026
@SubhamSinghal
SubhamSinghal marked this pull request as ready for review November 2, 2026 07:43
@jayzhan211
jayzhan211 added this pull request to the merge queue Nov 2, 2026
@jayzhan211

Copy link
Copy Markdown
Contributor

Thanks @SubhamSinghal @comphead !

Merged via the queue into apache:main with commit 198ab43 Nov 2, 2026
45 checks passed
@SubhamSinghal
SubhamSinghal deleted the pwmj-classic-batched-emission branch November 2, 2026 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt) v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants