Repository navigation
perf: skip probe hashing in the routed lookup when every map is an array map - #27324
Merged
Merged
Conversation
adriangb
marked this pull request as ready for review
November 11, 2026 19:00
adriangb
commented
Nov 11, 2026
…ray map An array map probes by key value, so when no partition holds a hash map the routed lookup computed per-row hashes that nothing read. The lookup now hashes the probe keys only when some partition is a hash map. The counting sort by partition runs either way, and the sorted hashes are gathered from the sorted rows only when hashes exist. A new test builds every partition as an array map and checks exact matches, including a null probe row. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Review asked for two changes. A `matches!` on `Map::HashMap` would silently treat a new `Map` variant as not needing hashes, so an exhaustive match makes the compiler flag that case. An empty `Vec` stood for "no hashes", which a reader could mistake for zero rows, so `Option<Vec<u64>>` states it directly. The hash map arm returns an internal error if it ever runs without hashes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The routed lookup carried `Option` hashes and a hash map arm that had to return an internal error when the hashes were missing. That state could not happen, but the types allowed it. Review asked to remove it. The lookup now collects the maps into `Option<Vec<Option<&ArrayMap>>>`, which is `Some` only when no partition holds a hash map. That path has no hashes and no hash map arm. The other path always computes hashes. Both paths share the counting sort (`sort_rows_by_partition`) and the per-partition probe loop (`probe_runs`). Behavior does not change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb
force-pushed
the
routed-lookup-skip-hashes
branch
from
November 12, 2026 06:46
99abca1 to
8e8462b
Compare
jayzhan211
approved these changes
Nov 12, 2026
Contributor
|
Thanks @adriangb ! |
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
Partitionedhash join with dense integer keys builds anArrayMapin every partition. Its dynamic filter then computes a hash for every probe row, but anArrayMaplooks keys up by value, so nothing reads those hashes. Skipping them makes the filter cheaper on every probe batch.What changes are included in this PR?
HashTableLookupExpr::evaluate_routedhashes the probe keys only when at least one partition is a hash table. The counting sort by partition runs either way.What is the testing strategy for this PR?
ArrayMapand checks exact matches, including a null probe row.force_hash_collisions.Are there any user-facing changes?
No.
🤖 Generated with Claude Code