Repository navigation
perf(pwmj): build output batches from buffered runs and one streamed … - #26287
Conversation
…gather, drop the per-row batch + coalescer
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
|
@comphead @jayzhan211 @kumarUjjawal Can you help in reviewing this PR. Thanks. |
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @SubhamSinghal , LGTM!
comphead
left a comment
There was a problem hiding this comment.
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 againstNestedLoopJoin) andfuzz_pwmj_matches_nested_loop(batch size 3,i32andf64, several partitions) already compare row for row. Only the batch layout assertions are new coverage.join_right_less_than_empty_bufferedhas no layout assertion, so aRIGHT JOINagainst an empty table in the matrix file would replace it. join_full_less_than_splits_suffixes_across_batchesandjoin_inner_less_than_one_row_completes_several_batchesboth split a buffered suffix acrossbatch_sizeinpush_matched. One of them is probably enough.- Every payload column in the new tests, the fuzz harness and the
pwmjbenchmark isInt32.copy_runsgoes throughMutableArrayDatafor every type, so aUtf8and aUtf8Viewpayload case would cover the paths that differ from fixed width.
Benchmarks
- The
pwmjsuite has anInt32payload 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 mostbatch_sizeentries, so it can be sized once likestreamed_indices.- The
OutputBufferdoc 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_batchcallspush_unmatched_bufferedtwice and each call can end with a partial chunk. Onecopy_runsover 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)?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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, |
There was a problem hiding this comment.
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.
| .columns() | ||
| .iter() | ||
| .map(|column| { | ||
| let data = column.to_data(); |
There was a problem hiding this comment.
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.
|
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 |
|
Thanks @SubhamSinghal @comphead ! |
Which issue does this PR close?
PiecewiseMergeJoinperformance for large tables #18221 (and the PiecewiseMergeJoin EPIC [EPIC]: MakePiecewiseMergeJoinwork in Datafusion #17427).Rationale for this change
On large inputs the classic
PiecewiseMergeJoin(Inner/Left/Right/Full) can be slower thanNestedLoopJoin. For example, before #25840,l JOIN r ON l.a < r.bover 100K×100K rows at the defaulttarget_partitionstook 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:takerepeating each matched streamed rowBatchCoalescermemmoveof every columnsum(the query itself)RecordBatch, allocations, coalescer bookkeeping)For every matched streamed row, the join takes
UInt32Array::from_value(row, count), gathers the streamed rowcounttimes, slices the buffered suffix, and builds aRecordBatchfrom them.BatchCoalescerthen 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 currentmainthe 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
BatchCoalesceris removed from this path.Matched rows:
(buffered start, len), paired with one streamed row index per output row.batch_sizerows are pending, or the stream batch ends:MutableArrayData::try_extend, which is amemcpyfor fixed-width types.takeper column.Rows that match nothing are emitted with null columns for the other side, in batches of at most
batch_size:Right/Full), which perf(pwmj):settle never-matching streamed rows against the buffered extreme… #25840 settles before the sort, are selected with onefilter_record_batchand emitted as slices of its output (push_unmatched_streamed). When every row of a stream batch is unmatched,filterreturns the input columns as a slice and copies nothing.Left/Full), in the final pass, are the prefix before themin_markedwatermark 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 batchBenchmarks
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 -- pwmjWhat is the testing strategy for this PR?
Six new unit tests in
classic_join.rs:join_full_less_than_splits_suffixes_across_batches: atbatch_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 thanbatch_size, which must be split into batches of at mostbatch_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 thanbatch_sizeplus 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 ANALYZEreports a smalleroutput_bytesforPiecewiseMergeJoin, because output batches no longer share the coalescer's buffers (explain_analyze.slt: 128.0 KB → 96.0 B).