-
Notifications
You must be signed in to change notification settings - Fork 1.9k
perf: optimize HashTableLookupExpr::evaluate
#19602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR optimizes HashTableLookupExpr::evaluate by replacing per-row hash table lookups with a batch processing API. The previous implementation made individual get_matched_indices calls for each row, causing unnecessary memory allocations and redundant computations. The new approach uses a single batch call that sets bits in a buffer for all matching hashes at once, resulting in notable performance improvements across multiple TPC-H queries (up to 1.13x faster on Q18).
Key Changes:
- Introduced
set_bits_if_existstrait method for batch hash lookups - Refactored
HashTableLookupExpr::evaluateto use the new batch API - Added comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
datafusion/physical-plan/src/joins/join_hash_map.rs |
Adds set_bits_if_exists trait method to JoinHashMapType and implements it for JoinHashMapU32 and JoinHashMapU64 with corresponding helper function and tests |
datafusion/physical-plan/src/joins/stream_join_utils.rs |
Implements set_bits_if_exists for PruningJoinHashMap to support the new batch lookup API |
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs |
Refactors HashTableLookupExpr::evaluate to use the new batch API instead of per-row lookups |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| self.hash_map | ||
| .set_bits_if_exists(hash_array.values(), buf.as_slice_mut()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use with_hashes / reuse hashes buffer instead of allocating a new one each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Reusing the hashes buffer now. Thanks!
|
Very nice! I added 2 suggestions which could maybe improve it even further. |
Yes that's per design. It's only used when the filter is pushed down into and evaluated row by row by the Parquet machinery. There's work to make that the default: #19477 |
…er: &mut [u8])` -> `contain_hashes(&self, hash_values: &[u64]) -> BooleanArray`
1e684a1 to
2ab1a49
Compare
Thanks for reviewing! I've applied both suggestions. Could you please take another look? @Dandandan |
|
|
||
| // Optimization: if hash_expr is HashExpr, compute hashes directly into callback | ||
| // to avoid redundant allocations and copies. | ||
| if let Some(hash_expr) = self.hash_expr.as_any().downcast_ref::<HashExpr>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this always the case? We can remove the hashexpr (only store the inner expressions) while it is constructed to simplify the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point. Storing the inner expressions directly is indeed a better approach, especially since the perfect hash join(which is being worked on in that separate pending PR) needs direct access to on_columns to identify the join key columns.
I initially considered changing HashTableLookupExpr.hash_expr to a concrete HashExpr type (which I've actually already done in another unmerged PR for perfect hash join). However, I think that accessing the columns through hash_expr.on_columns feels a bit clunky 😂.
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs
Outdated
Show resolved
Hide resolved
898905f to
97864c9
Compare
|
It might be interesting to re-run https://datafusion.apache.org/blog/2025/09/10/dynamic-filters/#hash-join-dynamic-filters and see if the numbers are even better now! |
I re-ran the benchmark( |
|
Thank you @UBarney! |
Which issue does this PR close?
Rationale for this change
The previous implementation of
HashTableLookupExpr::evaluaterelied on per-row calls toget_matched_indices, which incurred unnecessary performance overhead:Vecallocations and potential resizes, leading to pressure on the memory allocator.get_matched_indicestraverses the entire hash chain to find all matches, which is unnecessary when we only need to verify the existence of a key.Performance Results (TPC-H)
The following TPC-H results were obtained with
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS=true:Note that Q1 does not involve
HashJoin.Note on Configuration
Benchmarks were conducted with
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS=truebecauseHashTableLookupExpr::evaluateis NOT invoked under default settings.I manually added
dbg!(&num_rows)at L335 inpartitioned_hash_eval.rsand confirmed that the logic path is only triggered when this flag is enabled. Under default settings,HashTableLookupExpr::evaluateis not called; . I am uncertain if this current behavior is intentional.What changes are included in this PR?
JoinHashMapType::contain_hashes: A new trait method that processesa batch of hashes and updates a bitmask for existing keys.
HashTableLookupExpr::evaluate: Switched from per-row lookups tothe new batch API.
Are these changes tested?
Yes
Are there any user-facing changes?
NO