Conversation
maxburke
changed the title
Dandandan
reviewed
Jun 21, 2026
maxburke
force-pushed
the
join-avoid-concat
branch
from
June 22, 2026 19:20
4c0a041 to
e42c1e1
Compare
mbutrovich
self-requested a review
June 30, 2026 23:40
Contributor
|
Thanks for putting this together. I'm trying to reason about how it behaves across a wider range of build-side shapes than the 60M-row case in the description, and a few things aren't obvious to me yet. Mostly questions rather than objections. Sliced / over-allocated input batches. Cost now applies to every multi-batch build, not just huge ones. Outer joins lose Dictionary and nested columns. PWMJ still concatenates the key column. On the memory numbers themselves, I'd find it easier to reason about this framed in bytes and by partition mode rather than row count. The doubling and the i32 offset limit are both byte-driven, and they mainly bite in |
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing join-avoid-concat (6c55cb9) to 742361b (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing join-avoid-concat (6c55cb9) to 742361b (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing join-avoid-concat (6c55cb9) to 742361b (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
|
run benchmark tpch10 hj |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing join-avoid-concat (2289eee) to 166c040 (merge-base) diff using: tpch10 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing join-avoid-concat (2289eee) to 166c040 (merge-base) diff using: hj File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagehj — base (merge-base)
hj — branch
File an issue against this benchmark runner |
maxburke
force-pushed
the
join-avoid-concat
branch
from
July 6, 2026 20:49
2289eee to
e820c36
Compare
Contributor
Author
|
All tests now pass; anything else this needs from me? |
maxburke
force-pushed
the
join-avoid-concat
branch
from
July 9, 2026 16:36
e820c36 to
255a6e0
Compare
Contributor
Author
|
Sorry, @mbutrovich, I missed this among all the benchmark messages.
I believe they are fully packed?
Unfortunately I do not.
No; we do not have much dictionary or struct data. We have some list data but we haven't seen any performance regressions worth noting. I'm sorry, I don't really have the depth of experience with the internals of the join machinery to really answer your questions deeply :(. I just started attacking the problem where our profiles (and panic backtraces) were pointing. |
This avoids significant memory pressure on large queries.
maxburke
force-pushed
the
join-avoid-concat
branch
from
July 14, 2026 15:18
791a47b to
771f302
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23032 +/- ##
==========================================
- Coverage 80.91% 80.91% -0.01%
==========================================
Files 1103 1103
Lines 377219 377538 +319
Branches 377219 377538 +319
==========================================
+ Hits 305244 305500 +256
- Misses 53775 53804 +29
- Partials 18200 18234 +34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@mbutrovich @Dandandan What is the status of this PR from a Datafusion point of view? Is there concrete action that needs to be taken to get it into mergable shape? Or is it unacceptable for Datafusion? edit: if it's unacceptable I'd rather be told that so I can move on. If it needs work I'd like to know what work needs to be done so that I can start on that. It's this limbo state that's frustrating. |
Contributor
I don't believe this is acceptable until a deeper performance analysis is performed. As it currently stands, it raises a legitimate performance concern, but I am not convinced that the solution is not over-specialized to your use case. DataFusion needs to remain generally performant, and there are too many open questions in my previous comment that have not been explored. |
Contributor
Author
@mbutrovich Because I'm just a community member, maybe you could provide some more guidance on specifically what you are looking for in order to get this across the finish line? What would a more acceptable solution look like? As an end user I don't know what others are using; when you say "I am not convinced that the solution is not over-specialized to your use case" I am confused because I don't have any other frame of reference. From my point of view, my queries are no longer crashing, runtime memory is significantly reduced, and all the tests are passing. I don't know what "a deeper performance analysis" entails. You mention "it raises a legitimate performance concern", but what specifically is the metric being measured that is a concern? I look at the benchmarks above and I see some faster and some slower (which makes sense), I don't know which are unacceptably slower. I don't know what tradeoffs are acceptable. I don't know which benchmarks are more important than the others. Are all performance impacting changes unacceptable? (maybe cc @alamb for help) |
Contributor
|
Let me give this a deeper look again this week, and see if we're missing any benchmarks that might address some of the scenarios I'm concerned about. Thanks for your patience on this one, @maxburke! |
Contributor
|
run benchmark tpch10 hj |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing join-avoid-concat (9fa9da6) to 9facaaa (merge-base) diff using: tpch10 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing join-avoid-concat (9fa9da6) to 9facaaa (merge-base) diff using: hj File an issue against this benchmark runner |
Contributor
In my mind, changes that result in some queries getting faster and some getting slower is problematic because it may be experienced as a regression by some community members My reading of the benchmark results on this PR were inconclusive -- I am not sure after reviewing the results if this PR actually makes some query cases slower or if the results are just noise. So in terms of concrete steps forward I would suggest:
|
Contributor
|
I kicked off another benchmark and hopefully the results will give us some more info |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagehj — base (merge-base)
hj — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
Resolves conflicts in joins/utils.rs with upstream apache#23843 (refactor join-key equality filtering), which replaced the take + eq_dyn_null candidate-pair filtering with a JoinKeyComparator and a monomorphized single-column fast path, deleting eq_dyn_null that equal_rows_arr_multi depended on. equal_rows_arr_multi is rewritten on the new machinery so the hash join keeps upstream's optimization: a single build batch delegates to equal_rows_arr, multi-batch builds get a multi-batch variant of the single-column fast path (equal_rows_single_col_multi), and other key shapes use lazily-built per-batch JoinKeyComparators. The equality path no longer gathers build-side keys; interleave remains only in take_build_array for output batch building. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
We hit the same failure modes in a setup similar to DataFusion Comet (2x build-side peak from concat_batches, and the all-or-nothing single allocation) and ended up with an NLJ-scoped version of this change that we'd like to upstream: the build side is kept as target-sized chunks produced by arrow's BatchCoalescer, with with_biggest_coalesce_batch_size retaining already-large batches zero-copy, the visited bitmap indexed by global row numbers over prefix-sum chunk offsets, and the spill write path going through the same coalescer so the memory-limited replay reads back uniform chunks. On an 880MB build side (20M rows, inequality join, target_partitions=1), pristine main peaks at 1738MB RSS vs 899MB with chunks, and with --memory-limit 1g main completes while peaking at 1739MB because the concat output is never reserved in the pool. @mbutrovich the two scenarios you raised earlier measure as follows on that branch: a build side arriving as 250,000 8-row batches probes at parity with main (7.36s vs 7.20s medians over interleaved runs, ~1.6e10 pair evaluations) because the coalescer compacts to target size before probing ever starts, and sliced batches below half the target size are compacted the same way, while larger ones are retained zero-copy with the reservation charging get_array_memory_size of the full parent buffers, which is conservative; precise dedup-by-allocation accounting is a planned follow-up. Issue at #24819 and PR at #24820 extract just this operator, leaving hash join and piecewise merge to this PR or follow-ups — @maxburke happy to coordinate if a stepping-stone is useful here. |
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
7 participants
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?
Closes issue #23031
Rationale for this change
We run into two problems when operating on datasets with approximately 60 million rows:
What changes are included in this PR?
This removes record batch concatenation from several joins (hash join, nested loop join, piecewise merge join)
Are these changes tested?
Yes
Are there any user-facing changes?
I sure hope not! (no)