Skip to content

perf: adapt generic hash join allocation to key cardinality - #25592

Closed
QinXi-ai wants to merge 2 commits into
apache:mainfrom
QinXi-ai:codex/25392-adaptive-hash-index
Closed

QinXi-ai wants to merge 2 commits into
apache:mainfrom
QinXi-ai:codex/25392-adaptive-hash-index

Conversation

@QinXi-ai

@QinXi-ai QinXi-ai commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #25392.

This is a draft alternative to #25434, which addresses the same issue. It is not ready for merge.

Rationale for this change

Generic hash joins currently reserve lookup buckets for every build row even when many rows share a key. The row-index chain must still retain every row, but the lookup table only needs one entry per distinct hash. Reserving unused buckets can reject joins that would otherwise fit in a bounded memory pool.

This version retains hashbrown's existing HashTable, grows its lookup index as distinct hashes arrive, and accounts for the old and new tables during growth. It avoids a custom compact table and post-build compaction or rebuild. It still has a sampling heuristic and Rust trait/API changes, so I do not claim that it is simpler overall than #25434. The alternative remains useful for comparing these tradeoffs.

What changes are included in this PR?

  • Start reservation-backed generic maps with a small lookup table while retaining one row-chain slot per build row.
  • Reserve before initial allocation and before growth, including hashbrown's trailing control group and the temporary old-plus-new table peak. Settle the reservation to retained storage after growth.
  • Update an existing hash's chain without requesting growth when the table is full.
  • Sample up to 1,024 already-computed hashes with bounded stack space to pre-size likely high-cardinality batches. A denied speculative reservation falls back to incremental growth.
  • Propagate allocation errors through insertion callers. Generic null-aware scope maps use the same reservation-backed constructor, and the current NullAwareMode handling for correlated NOT IN is preserved.
  • Integrate the build-batch and computed-key memory reservations added in fix: reserve the concatenated build batch and computed join keys in HashJoinExec #25496. The generic map now charges its final size before build-batch concatenation; the new memory-limit test estimates lookup buckets from distinct hashes while retaining a chain slot for every build row.
  • Add allocation, duplicate-chain, sampling, and real string-key join regressions plus a Criterion build benchmark.

Prepared builds retain a fully preallocated lookup table charged to their single durable reservation. Applying adaptive map ownership there would underreport reserved_bytes() and add a growth peak on top of the admitted concatenation copy; the existing prepared-build regression tests cover both contracts. The specialized perfect-hash ArrayMap path is unchanged. This adds no spilling behavior. The upstream build-batch concatenation and computed-key reservations are retained, but this PR does not independently validate all payload-memory accounting. build_mem_used reports retained map storage, not the temporary reservation peak or process RSS. The allocation estimate is tied to hashbrown 0.17's layout and is checked against actual allocation sizes in tests.

What is the testing strategy for this PR?

Unit tests cover U32/U64 index widths, duplicate updates at full capacity, reservation-before-allocation, old/new overlap, rejected hints, cleanup, repeated and periodic hashes, and string-key HashJoin behavior in CollectLeft and Partitioned modes.

Validated on October 2, 2026 after merging main at 4d167a1, PR head 0697bc3 (Windows x64, Rust 1.98.1):

  • cargo fmt --all --check, the repository Markdown formatter and git diff --check origin/main: passed.
  • cargo clippy --offline --locked --all-targets --all-features -- -D warnings: passed, without lint allowances.
  • cargo test --offline --locked -p datafusion-physical-plan --lib: 2,413 passed, 0 failed.
  • All 15 prepared-build tests pass. Before the compatibility fix, three existing tests failed: retained reservation reporting, IN-list/shared-buffer accounting and the bounded small-batch concatenation case. The fix preserves the original upfront allocation contract for prepared builds; no test budgets or assertions were weakened.

The complete extended workspace suite and performance benchmarks were not rerun for this maintenance update. Earlier Windows validation had broader environment/baseline blockers; those historical attempts are not a full pass for this head. The PR remains a draft alternative pending upstream CI, comparative performance evidence and maintainer design feedback.

The following benchmark measurements are historical results from the previous base, 95bb0a0; they have not been rerun against 4d167a1 and do not establish performance for the current head. The native Criterion benchmark builds 200,000 UTF-8 rows in either 8,192-row batches or one batch and probes with one absent key. The identical benchmark source was built separately against that base and the prior PR head, with separate target directories. Input generation is outside timing. Two Windows runs used 10 samples / 1 s warmup / 2 s measurement and a focused recheck with 20 samples / 2 s warmup / 5 s measurement. Median times in milliseconds, first run / recheck:

Distribution / batch size 95bb0a0 Prior PR head
8 keys / 8,192 2.09 / 1.81 1.42 / 1.67
unique / 8,192 7.72 / 6.82 5.42 / 6.42
8 keys / 200,000 2.08 / 2.39 1.85 / 2.10
unique / 200,000 7.07 / 6.31 7.39 / 6.38

These are preliminary Windows measurements with visible run-to-run variation. They show a low-cardinality benefit in this workload and do not reproduce a large high-cardinality regression in the native benchmark, but they do not establish performance across distributions or platforms. An earlier, different map-only microbenchmark on a pre-rebase base measured a 14–15% unique-key slowdown; it remains historical evidence, not a result for this current branch. More complete CI and performance validation are needed before leaving Draft.

To run the benchmark:

cargo bench --locked -p datafusion-physical-plan --features test_utils \
  --bench hash_join_semi_anti -- 'generic_hash_join_build/'

For a base comparison, copy this benchmark file to the base checkout and use separate target directories.

Are there any user-facing changes?

Duplicate-heavy generic hash joins can use smaller memory pools while preserving all matching rows. No SQL syntax or configuration changes are introduced.

Rust API changes: JoinHashMapType::update_from_iter and the shared insertion helper now return Result<()>; the trait gains a required size method and a default reserve_from_batch method, and the insertion helper accepts an optional reservation. Downstream implementations or callers of these public, primarily internal interfaces need adjustment. Please apply the api change label.

This contribution is AI-assisted.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 22, 2026
@QinXi-ai
QinXi-ai force-pushed the codex/25392-adaptive-hash-index branch from 150f3df to 8783f23 Compare September 24, 2026 03:51
@QinXi-ai
QinXi-ai force-pushed the codex/25392-adaptive-hash-index branch from 8783f23 to 04430bb Compare September 24, 2026 14:01
@codecov-commenter

codecov-commenter commented Oct 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.61938% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.61%. Comparing base (4d167a1) to head (0697bc3).
⚠️ Report is 357 commits behind head on main.

Files with missing lines Patch % Lines
...atafusion/physical-plan/src/joins/join_hash_map.rs 92.63% 17 Missing and 16 partials ⚠️
...tafusion/physical-plan/src/joins/hash_join/exec.rs 83.67% 7 Missing and 9 partials ⚠️
...physical-plan/src/joins/hash_join/exec/prepared.rs 72.72% 3 Missing and 3 partials ⚠️
datafusion/physical-plan/src/joins/utils.rs 40.00% 0 Missing and 3 partials ⚠️
...usion/physical-plan/src/joins/stream_join_utils.rs 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25592      +/-   ##
==========================================
+ Coverage   82.60%   82.61%   +0.01%     
==========================================
  Files        1145     1145              
  Lines      444057   444584     +527     
  Branches   444057   444584     +527     
==========================================
+ Hits       366796   367294     +498     
+ Misses      54996    54994       -2     
- Partials    22265    22296      +31     

☔ 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.

@alamb

alamb commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution! I am closing this under DataFusion's AI Dump policy.

https://datafusion.apache.org/contributor-guide/ai-policy.html#what-is-an-ai-dump-and-why-it-is-not-helpful

This has the hallmarks of AI code that was not reviewed by the author: a 900-line alternative implementation for an issue already being worked elsewhere, with boilerplate local validation text duplicated across the author's PRs.

If you would like to continue, please open a focused PR in your own words that you have reviewed and understand, and we are happy to help with review.

@alamb alamb closed this Oct 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce generic hash join bucket allocation for low-cardinality build keys

3 participants