You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "UBarney (via GitHub)" <gi...@apache.org> on 2026/01/02 08:19:40 UTC
[PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
UBarney opened a new pull request, #19602:
URL: https://github.com/apache/datafusion/pull/19602
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
The previous implementation of `HashTableLookupExpr::evaluate` relied on per-row calls to `get_matched_indices`, which introduced significant performance bottlenecks:
1. **High Memory Overhead**: Each per-row call triggered small `Vec` allocations and potential resizes, leading to excessive pressure on the memory allocator.
2. **Redundant Computation**: `get_matched_indices` traverses 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`:**
```
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ baseline@9a9ff ┃ optimized ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1 │ 679.51 ms │ 728.06 ms │ 1.07x slower │
│ QQuery 2 │ 388.33 ms │ 384.11 ms │ no change │
│ QQuery 3 │ 864.38 ms │ 856.27 ms │ no change │
│ QQuery 4 │ 458.46 ms │ 468.26 ms │ no change │
│ QQuery 5 │ 1614.26 ms │ 1525.65 ms │ +1.06x faster │
│ QQuery 6 │ 611.20 ms │ 610.06 ms │ no change │
│ QQuery 7 │ 950.39 ms │ 940.13 ms │ no change │
│ QQuery 8 │ 1214.86 ms │ 1218.21 ms │ no change │
│ QQuery 9 │ 2657.61 ms │ 2482.09 ms │ +1.07x faster │
│ QQuery 10 │ 1050.70 ms │ 1001.96 ms │ no change │
│ QQuery 11 │ 383.92 ms │ 347.27 ms │ +1.11x faster │
│ QQuery 12 │ 963.14 ms │ 920.78 ms │ no change │
│ QQuery 13 │ 473.68 ms │ 480.97 ms │ no change │
│ QQuery 14 │ 363.36 ms │ 345.27 ms │ no change │
│ QQuery 15 │ 960.56 ms │ 955.05 ms │ no change │
│ QQuery 16 │ 281.95 ms │ 267.34 ms │ +1.05x faster │
│ QQuery 17 │ 5306.43 ms │ 4983.21 ms │ +1.06x faster │
│ QQuery 18 │ 3415.11 ms │ 3016.52 ms │ +1.13x faster │
│ QQuery 19 │ 761.67 ms │ 759.49 ms │ no change │
│ QQuery 20 │ 650.20 ms │ 642.40 ms │ no change │
│ QQuery 21 │ 3111.85 ms │ 2833.05 ms │ +1.10x faster │
│ QQuery 22 │ 141.75 ms │ 143.06 ms │ no change │
└──────────────┴────────────────┴────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (baseline@9a9ff) │ 27303.30ms │
│ Total Time (optimized) │ 25909.21ms │
│ Average Time (baseline@9a9ff) │ 1241.06ms │
│ Average Time (optimized) │ 1177.69ms │
│ Queries Faster │ 7 │
│ Queries Slower │ 1 │
│ Queries with No Change │ 14 │
│ Queries with Failure │ 0 │
└───────────────────────────────┴────────────┘
```
Note that Q1 does not involve `HashJoin`.
### Note on Configuration
Benchmarks were conducted with `DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS=true` because `HashTableLookupExpr::evaluate` is **NOT** invoked under default settings.
I manually added `dbg!(&num_rows)` at [L335 in `partitioned_hash_eval.rs`](https://github.com/apache/datafusion/blob/9a9ff8d6162b7391736b0b7c82c00cb35b0652a1/datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs#L335) and confirmed that the logic path is only triggered when this flag is enabled. Under default settings, `HashTableLookupExpr::evaluate` is not called; . I am uncertain if this current behavior is intentional.
## What changes are included in this PR?
- Added `JoinHashMapType::set_bits_if_exists`: A new trait method that processes
a batch of hashes and updates a bitmask for existing keys.
- Refactored `HashTableLookupExpr::evaluate`: Switched from per-row lookups to
the new batch API.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
NO
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2659556702
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -327,12 +329,24 @@ impl PhysicalExpr for HashTableLookupExpr {
Ok(false)
}
- fn evaluate(
- &self,
- batch: &arrow::record_batch::RecordBatch,
- ) -> Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let num_rows = batch.num_rows();
+ // 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>() {
Review Comment:
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](https://github.com/apache/datafusion/pull/19411/changes#diff-c89cc13760a9e27ff7901446ad0982fab905ae292b57803b53f749603168c7b6R361-R362)) 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](https://github.com/apache/datafusion/pull/19411/changes#diff-c89cc13760a9e27ff7901446ad0982fab905ae292b57803b53f749603168c7b6R222)). However, I think that accessing the columns through `hash_expr.on_columns` feels a bit clunky 😂.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2657591862
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -343,16 +343,8 @@ impl PhysicalExpr for HashTableLookupExpr {
// Check each hash against the hash table
let mut buf = MutableBuffer::from_len_zeroed(bit_util::ceil(num_rows, 8));
- for (idx, hash_value) in hash_array.values().iter().enumerate() {
- // Use get_matched_indices to check - if it returns any indices, the hash exists
- let (matched_indices, _) = self
- .hash_map
- .get_matched_indices(Box::new(std::iter::once((idx, hash_value))), None);
-
- if !matched_indices.is_empty() {
- bit_util::set_bit(buf.as_slice_mut(), idx);
- }
- }
+ self.hash_map
+ .set_bits_if_exists(hash_array.values(), buf.as_slice_mut());
Review Comment:
I think we could use `with_hashes` / reuse hashes buffer instead of allocating a new one each time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3709172542
> 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(`SELECT *
FROM small_table JOIN large_table ON small_table.k = large_table.k
WHERE small_table.v >= 50;`), but the execution time remains almost unchanged at around 7ms. It seems the min-max filter (`k@0 >= 50 AND k@0 <= 1000`) is already filtering out the majority of the data, which is likely why we don't see a significant difference.
@adriangb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705177012
Very nice! I added 2 suggestions which could maybe improve it even further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2659721105
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -327,12 +329,24 @@ impl PhysicalExpr for HashTableLookupExpr {
Ok(false)
}
- fn evaluate(
- &self,
- batch: &arrow::record_batch::RecordBatch,
- ) -> Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let num_rows = batch.num_rows();
+ // 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>() {
+ let keys_values = hash_expr.evaluate_on_columns(batch)?;
Review Comment:
Ok makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3708179452
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3714075170
Thank you @UBarney!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705103825
🤖 `./gh_compare_branch.sh` [gh_compare_branch.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_branch.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing opt_htlExpr_eval (1e684a1e2ab97129905b2176cfa677d8a3ef828c) to 9a9ff8d6162b7391736b0b7c82c00cb35b0652a1 [diff](https://github.com/apache/datafusion/compare/9a9ff8d6162b7391736b0b7c82c00cb35b0652a1..1e684a1e2ab97129905b2176cfa677d8a3ef828c) using: tpch
Results will be posted here when complete
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705117937
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and opt_htlExpr_eval
--------------------
Benchmark tpcds_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ HEAD ┃ opt_htlExpr_eval ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1 │ 62.24 ms │ 60.58 ms │ no change │
│ QQuery 2 │ 204.55 ms │ 210.38 ms │ no change │
│ QQuery 3 │ 157.06 ms │ 160.46 ms │ no change │
│ QQuery 4 │ 1898.21 ms │ 1867.16 ms │ no change │
│ QQuery 5 │ 265.85 ms │ 271.18 ms │ no change │
│ QQuery 6 │ 1434.03 ms │ 1426.55 ms │ no change │
│ QQuery 7 │ 495.16 ms │ 501.26 ms │ no change │
│ QQuery 8 │ 168.91 ms │ 168.12 ms │ no change │
│ QQuery 9 │ 284.30 ms │ 271.84 ms │ no change │
│ QQuery 10 │ 162.42 ms │ 167.90 ms │ no change │
│ QQuery 11 │ 1284.10 ms │ 1257.35 ms │ no change │
│ QQuery 12 │ 70.03 ms │ 70.33 ms │ no change │
│ QQuery 13 │ 540.21 ms │ 549.33 ms │ no change │
│ QQuery 14 │ 1910.85 ms │ 1918.95 ms │ no change │
│ QQuery 15 │ 27.03 ms │ 27.87 ms │ no change │
│ QQuery 16 │ 55.89 ms │ 55.80 ms │ no change │
│ QQuery 17 │ 346.90 ms │ 348.66 ms │ no change │
│ QQuery 18 │ 183.10 ms │ 189.33 ms │ no change │
│ QQuery 19 │ 222.28 ms │ 222.41 ms │ no change │
│ QQuery 20 │ 22.00 ms │ 22.37 ms │ no change │
│ QQuery 21 │ 33.47 ms │ 32.84 ms │ no change │
│ QQuery 22 │ 755.61 ms │ 749.83 ms │ no change │
│ QQuery 23 │ 1826.74 ms │ 1819.94 ms │ no change │
│ QQuery 24 │ 623.21 ms │ 640.60 ms │ no change │
│ QQuery 25 │ 510.54 ms │ 508.93 ms │ no change │
│ QQuery 26 │ 120.20 ms │ 122.75 ms │ no change │
│ QQuery 27 │ 485.16 ms │ 494.31 ms │ no change │
│ QQuery 28 │ 295.98 ms │ 301.26 ms │ no change │
│ QQuery 29 │ 436.69 ms │ 434.77 ms │ no change │
│ QQuery 30 │ 58.86 ms │ 64.47 ms │ 1.10x slower │
│ QQuery 31 │ 285.91 ms │ 297.37 ms │ no change │
│ QQuery 32 │ 76.41 ms │ 76.42 ms │ no change │
│ QQuery 33 │ 189.85 ms │ 191.13 ms │ no change │
│ QQuery 34 │ 154.40 ms │ 154.81 ms │ no change │
│ QQuery 35 │ 166.86 ms │ 161.40 ms │ no change │
│ QQuery 36 │ 298.37 ms │ 294.88 ms │ no change │
│ QQuery 37 │ 257.83 ms │ 255.43 ms │ no change │
│ QQuery 38 │ 141.52 ms │ 142.90 ms │ no change │
│ QQuery 39 │ 204.62 ms │ 201.49 ms │ no change │
│ QQuery 40 │ 195.55 ms │ 186.90 ms │ no change │
│ QQuery 41 │ 16.15 ms │ 17.09 ms │ 1.06x slower │
│ QQuery 42 │ 140.89 ms │ 141.95 ms │ no change │
│ QQuery 43 │ 123.60 ms │ 122.37 ms │ no change │
│ QQuery 44 │ 14.93 ms │ 14.52 ms │ no change │
│ QQuery 45 │ 81.17 ms │ 81.63 ms │ no change │
│ QQuery 46 │ 313.29 ms │ 309.97 ms │ no change │
│ QQuery 47 │ 1104.35 ms │ 1091.93 ms │ no change │
│ QQuery 48 │ 404.97 ms │ 412.47 ms │ no change │
│ QQuery 49 │ 358.29 ms │ 352.94 ms │ no change │
│ QQuery 50 │ 337.45 ms │ 328.37 ms │ no change │
│ QQuery 51 │ 296.30 ms │ 287.18 ms │ no change │
│ QQuery 52 │ 142.04 ms │ 143.23 ms │ no change │
│ QQuery 53 │ 145.58 ms │ 145.70 ms │ no change │
│ QQuery 54 │ 200.80 ms │ 203.13 ms │ no change │
│ QQuery 55 │ 141.02 ms │ 142.01 ms │ no change │
│ QQuery 56 │ 185.70 ms │ 188.97 ms │ no change │
│ QQuery 57 │ 285.08 ms │ 284.26 ms │ no change │
│ QQuery 58 │ 501.30 ms │ 514.03 ms │ no change │
│ QQuery 59 │ 290.68 ms │ 291.48 ms │ no change │
│ QQuery 60 │ 193.41 ms │ 196.51 ms │ no change │
│ QQuery 61 │ 232.44 ms │ 232.41 ms │ no change │
│ QQuery 62 │ 1253.85 ms │ 1237.61 ms │ no change │
│ QQuery 63 │ 148.26 ms │ 148.92 ms │ no change │
│ QQuery 64 │ 1129.00 ms │ 1140.22 ms │ no change │
│ QQuery 65 │ 347.06 ms │ 346.43 ms │ no change │
│ QQuery 66 │ 395.71 ms │ 372.35 ms │ +1.06x faster │
│ QQuery 67 │ 547.72 ms │ 540.24 ms │ no change │
│ QQuery 68 │ 367.18 ms │ 367.08 ms │ no change │
│ QQuery 69 │ 161.33 ms │ 161.59 ms │ no change │
│ QQuery 70 │ 498.57 ms │ 497.93 ms │ no change │
│ QQuery 71 │ 178.57 ms │ 181.86 ms │ no change │
│ QQuery 72 │ 2418.41 ms │ 2399.37 ms │ no change │
│ QQuery 73 │ 150.81 ms │ 150.48 ms │ no change │
│ QQuery 74 │ 788.59 ms │ 785.08 ms │ no change │
│ QQuery 75 │ 380.01 ms │ 369.38 ms │ no change │
│ QQuery 76 │ 180.71 ms │ 181.17 ms │ no change │
│ QQuery 77 │ 263.73 ms │ 263.71 ms │ no change │
│ QQuery 78 │ 936.97 ms │ 905.80 ms │ no change │
│ QQuery 79 │ 320.20 ms │ 315.55 ms │ no change │
│ QQuery 80 │ 488.56 ms │ 492.11 ms │ no change │
│ QQuery 81 │ 39.39 ms │ 41.00 ms │ no change │
│ QQuery 82 │ 294.52 ms │ 288.94 ms │ no change │
│ QQuery 83 │ 68.46 ms │ 69.55 ms │ no change │
│ QQuery 84 │ 70.99 ms │ 60.65 ms │ +1.17x faster │
│ QQuery 85 │ 211.20 ms │ 225.10 ms │ 1.07x slower │
│ QQuery 86 │ 54.26 ms │ 54.72 ms │ no change │
│ QQuery 87 │ 138.64 ms │ 138.28 ms │ no change │
│ QQuery 88 │ 238.91 ms │ 235.76 ms │ no change │
│ QQuery 89 │ 162.72 ms │ 163.34 ms │ no change │
│ QQuery 90 │ 34.67 ms │ 34.18 ms │ no change │
│ QQuery 91 │ 95.57 ms │ 92.50 ms │ no change │
│ QQuery 92 │ 76.09 ms │ 77.29 ms │ no change │
│ QQuery 93 │ 262.27 ms │ 264.86 ms │ no change │
│ QQuery 94 │ 83.96 ms │ 82.61 ms │ no change │
│ QQuery 95 │ 251.47 ms │ 255.73 ms │ no change │
│ QQuery 96 │ 108.92 ms │ 107.31 ms │ no change │
│ QQuery 97 │ 178.34 ms │ 181.40 ms │ no change │
│ QQuery 98 │ 232.50 ms │ 229.18 ms │ no change │
│ QQuery 99 │ 14126.66 ms │ 14058.76 ms │ no change │
└──────────────┴─────────────┴──────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (HEAD) │ 50107.14ms │
│ Total Time (opt_htlExpr_eval) │ 49920.47ms │
│ Average Time (HEAD) │ 506.13ms │
│ Average Time (opt_htlExpr_eval) │ 504.25ms │
│ Queries Faster │ 2 │
│ Queries Slower │ 3 │
│ Queries with No Change │ 94 │
│ Queries with Failure │ 0 │
└─────────────────────────────────┴────────────┘
```
</p>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705103710
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and opt_htlExpr_eval
--------------------
Benchmark clickbench_extended.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Query ┃ HEAD ┃ opt_htlExpr_eval ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ QQuery 0 │ 2422.42 ms │ 2446.43 ms │ no change │
│ QQuery 1 │ 923.93 ms │ 960.35 ms │ no change │
│ QQuery 2 │ 1890.08 ms │ 1849.77 ms │ no change │
│ QQuery 3 │ 1183.30 ms │ 1145.73 ms │ no change │
│ QQuery 4 │ 2293.14 ms │ 2292.15 ms │ no change │
│ QQuery 5 │ 28170.75 ms │ 28205.88 ms │ no change │
│ QQuery 6 │ 4006.35 ms │ 3920.25 ms │ no change │
│ QQuery 7 │ 3819.45 ms │ 3647.05 ms │ no change │
└──────────────┴─────────────┴──────────────────┴───────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (HEAD) │ 44709.43ms │
│ Total Time (opt_htlExpr_eval) │ 44467.60ms │
│ Average Time (HEAD) │ 5588.68ms │
│ Average Time (opt_htlExpr_eval) │ 5558.45ms │
│ Queries Faster │ 0 │
│ Queries Slower │ 0 │
│ Queries with No Change │ 8 │
│ Queries with Failure │ 0 │
└─────────────────────────────────┴────────────┘
--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ HEAD ┃ opt_htlExpr_eval ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0 │ 1.39 ms │ 1.44 ms │ no change │
│ QQuery 1 │ 48.70 ms │ 49.88 ms │ no change │
│ QQuery 2 │ 136.60 ms │ 133.80 ms │ no change │
│ QQuery 3 │ 157.75 ms │ 156.01 ms │ no change │
│ QQuery 4 │ 1076.54 ms │ 1107.30 ms │ no change │
│ QQuery 5 │ 1385.70 ms │ 1393.00 ms │ no change │
│ QQuery 6 │ 1.53 ms │ 1.45 ms │ +1.05x faster │
│ QQuery 7 │ 59.06 ms │ 57.24 ms │ no change │
│ QQuery 8 │ 1455.81 ms │ 1473.81 ms │ no change │
│ QQuery 9 │ 1822.35 ms │ 1792.80 ms │ no change │
│ QQuery 10 │ 344.96 ms │ 348.61 ms │ no change │
│ QQuery 11 │ 398.60 ms │ 403.28 ms │ no change │
│ QQuery 12 │ 1306.04 ms │ 1279.75 ms │ no change │
│ QQuery 13 │ 1971.55 ms │ 1970.87 ms │ no change │
│ QQuery 14 │ 1250.59 ms │ 1242.51 ms │ no change │
│ QQuery 15 │ 1298.96 ms │ 1283.13 ms │ no change │
│ QQuery 16 │ 2619.86 ms │ 2598.23 ms │ no change │
│ QQuery 17 │ 2572.52 ms │ 2601.41 ms │ no change │
│ QQuery 18 │ 5332.65 ms │ 4898.52 ms │ +1.09x faster │
│ QQuery 19 │ 122.75 ms │ 120.48 ms │ no change │
│ QQuery 20 │ 1940.84 ms │ 1840.11 ms │ +1.05x faster │
│ QQuery 21 │ 2249.48 ms │ 2121.10 ms │ +1.06x faster │
│ QQuery 22 │ 3821.11 ms │ 3608.66 ms │ +1.06x faster │
│ QQuery 23 │ 20381.18 ms │ 12026.73 ms │ +1.69x faster │
│ QQuery 24 │ 217.24 ms │ 201.99 ms │ +1.08x faster │
│ QQuery 25 │ 476.76 ms │ 449.38 ms │ +1.06x faster │
│ QQuery 26 │ 230.72 ms │ 201.01 ms │ +1.15x faster │
│ QQuery 27 │ 2751.10 ms │ 2680.22 ms │ no change │
│ QQuery 28 │ 23772.33 ms │ 24485.95 ms │ no change │
│ QQuery 29 │ 985.33 ms │ 987.03 ms │ no change │
│ QQuery 30 │ 1352.03 ms │ 1302.36 ms │ no change │
│ QQuery 31 │ 1396.31 ms │ 1388.37 ms │ no change │
│ QQuery 32 │ 4927.66 ms │ 4776.98 ms │ no change │
│ QQuery 33 │ 5684.10 ms │ 5481.19 ms │ no change │
│ QQuery 34 │ 5697.93 ms │ 5748.90 ms │ no change │
│ QQuery 35 │ 1988.18 ms │ 1889.92 ms │ no change │
│ QQuery 36 │ 65.40 ms │ 64.91 ms │ no change │
│ QQuery 37 │ 43.79 ms │ 44.37 ms │ no change │
│ QQuery 38 │ 65.48 ms │ 64.83 ms │ no change │
│ QQuery 39 │ 106.21 ms │ 99.82 ms │ +1.06x faster │
│ QQuery 40 │ 26.99 ms │ 24.99 ms │ +1.08x faster │
│ QQuery 41 │ 21.60 ms │ 22.04 ms │ no change │
│ QQuery 42 │ 19.80 ms │ 19.48 ms │ no change │
└──────────────┴─────────────┴──────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━┩
│ Total Time (HEAD) │ 101585.48ms │
│ Total Time (opt_htlExpr_eval) │ 92443.87ms │
│ Average Time (HEAD) │ 2362.45ms │
│ Average Time (opt_htlExpr_eval) │ 2149.86ms │
│ Queries Faster │ 11 │
│ Queries Slower │ 0 │
│ Queries with No Change │ 32 │
│ Queries with Failure │ 0 │
└─────────────────────────────────┴─────────────┘
--------------------
Benchmark tpch_mem_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ HEAD ┃ opt_htlExpr_eval ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1 │ 119.78 ms │ 117.73 ms │ no change │
│ QQuery 2 │ 29.15 ms │ 28.32 ms │ no change │
│ QQuery 3 │ 37.48 ms │ 37.33 ms │ no change │
│ QQuery 4 │ 29.49 ms │ 28.71 ms │ no change │
│ QQuery 5 │ 87.47 ms │ 86.95 ms │ no change │
│ QQuery 6 │ 23.60 ms │ 19.58 ms │ +1.21x faster │
│ QQuery 7 │ 228.64 ms │ 221.08 ms │ no change │
│ QQuery 8 │ 31.49 ms │ 32.98 ms │ no change │
│ QQuery 9 │ 97.83 ms │ 97.96 ms │ no change │
│ QQuery 10 │ 62.00 ms │ 62.28 ms │ no change │
│ QQuery 11 │ 17.83 ms │ 17.11 ms │ no change │
│ QQuery 12 │ 50.79 ms │ 50.19 ms │ no change │
│ QQuery 13 │ 47.70 ms │ 49.34 ms │ no change │
│ QQuery 14 │ 13.72 ms │ 14.09 ms │ no change │
│ QQuery 15 │ 24.21 ms │ 24.38 ms │ no change │
│ QQuery 16 │ 24.59 ms │ 24.48 ms │ no change │
│ QQuery 17 │ 146.15 ms │ 146.67 ms │ no change │
│ QQuery 18 │ 276.22 ms │ 274.25 ms │ no change │
│ QQuery 19 │ 40.27 ms │ 37.53 ms │ +1.07x faster │
│ QQuery 20 │ 51.72 ms │ 49.16 ms │ no change │
│ QQuery 21 │ 303.13 ms │ 290.61 ms │ no change │
│ QQuery 22 │ 17.44 ms │ 17.44 ms │ no change │
└──────────────┴───────────┴──────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (HEAD) │ 1760.70ms │
│ Total Time (opt_htlExpr_eval) │ 1728.18ms │
│ Average Time (HEAD) │ 80.03ms │
│ Average Time (opt_htlExpr_eval) │ 78.55ms │
│ Queries Faster │ 2 │
│ Queries Slower │ 0 │
│ Queries with No Change │ 20 │
│ Queries with Failure │ 0 │
└─────────────────────────────────┴───────────┘
```
</p>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2658958831
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -343,16 +343,8 @@ impl PhysicalExpr for HashTableLookupExpr {
// Check each hash against the hash table
let mut buf = MutableBuffer::from_len_zeroed(bit_util::ceil(num_rows, 8));
- for (idx, hash_value) in hash_array.values().iter().enumerate() {
- // Use get_matched_indices to check - if it returns any indices, the hash exists
- let (matched_indices, _) = self
- .hash_map
- .get_matched_indices(Box::new(std::iter::once((idx, hash_value))), None);
-
- if !matched_indices.is_empty() {
- bit_util::set_bit(buf.as_slice_mut(), idx);
- }
- }
+ self.hash_map
Review Comment:
Updated to use `BooleanBuffer::collect_bool`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2659557626
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -327,12 +329,24 @@ impl PhysicalExpr for HashTableLookupExpr {
Ok(false)
}
- fn evaluate(
- &self,
- batch: &arrow::record_batch::RecordBatch,
- ) -> Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let num_rows = batch.num_rows();
+ // 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>() {
+ let keys_values = hash_expr.evaluate_on_columns(batch)?;
Review Comment:
No, it doesn't compute the hashes. `evaluate_on_columns` only executes `c.evaluate(batch)?.into_array` for each column.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705104842
🤖 `./gh_compare_branch.sh` [gh_compare_branch.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_branch.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing opt_htlExpr_eval (1e684a1e2ab97129905b2176cfa677d8a3ef828c) to 9a9ff8d6162b7391736b0b7c82c00cb35b0652a1 [diff](https://github.com/apache/datafusion/compare/9a9ff8d6162b7391736b0b7c82c00cb35b0652a1..1e684a1e2ab97129905b2176cfa677d8a3ef828c) using: tpcds
Results will be posted here when complete
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705072409
run benchmark tpcds
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2658990690
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -327,12 +329,24 @@ impl PhysicalExpr for HashTableLookupExpr {
Ok(false)
}
- fn evaluate(
- &self,
- batch: &arrow::record_batch::RecordBatch,
- ) -> Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let num_rows = batch.num_rows();
+ // 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>() {
Review Comment:
Isn't this always the case? We can remove the hashexpr (only store the inner expressions) while it is constructed to simplify the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3706411444
> Under default settings, `HashTableLookupExpr::evaluate` is not called; . I am uncertain if this current behavior is intentional.
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705013270
run benchmarks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705014057
run benchmark tpch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705013801
🤖 `./gh_compare_branch.sh` [gh_compare_branch.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_branch.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing opt_htlExpr_eval (1e684a1e2ab97129905b2176cfa677d8a3ef828c) to 9a9ff8d6162b7391736b0b7c82c00cb35b0652a1 [diff](https://github.com/apache/datafusion/compare/9a9ff8d6162b7391736b0b7c82c00cb35b0652a1..1e684a1e2ab97129905b2176cfa677d8a3ef828c) using: tpch_mem clickbench_partitioned clickbench_extended
Results will be posted here when complete
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2658992146
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -327,12 +329,24 @@ impl PhysicalExpr for HashTableLookupExpr {
Ok(false)
}
- fn evaluate(
- &self,
- batch: &arrow::record_batch::RecordBatch,
- ) -> Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let num_rows = batch.num_rows();
+ // 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>() {
+ let keys_values = hash_expr.evaluate_on_columns(batch)?;
Review Comment:
Doesnt this already compute the hashes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2657486682
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -343,16 +343,8 @@ impl PhysicalExpr for HashTableLookupExpr {
// Check each hash against the hash table
let mut buf = MutableBuffer::from_len_zeroed(bit_util::ceil(num_rows, 8));
- for (idx, hash_value) in hash_array.values().iter().enumerate() {
- // Use get_matched_indices to check - if it returns any indices, the hash exists
- let (matched_indices, _) = self
- .hash_map
- .get_matched_indices(Box::new(std::iter::once((idx, hash_value))), None);
-
- if !matched_indices.is_empty() {
- bit_util::set_bit(buf.as_slice_mut(), idx);
- }
- }
+ self.hash_map
Review Comment:
Instead of initializing here and passing a slice, it could use `BooleanBuffer::collect_bool` (faster).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3705104791
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and opt_htlExpr_eval
--------------------
Benchmark tpch_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ HEAD ┃ opt_htlExpr_eval ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1 │ 195.39 ms │ 197.51 ms │ no change │
│ QQuery 2 │ 90.60 ms │ 93.01 ms │ no change │
│ QQuery 3 │ 125.40 ms │ 121.41 ms │ no change │
│ QQuery 4 │ 75.59 ms │ 77.17 ms │ no change │
│ QQuery 5 │ 179.42 ms │ 171.43 ms │ no change │
│ QQuery 6 │ 67.34 ms │ 68.70 ms │ no change │
│ QQuery 7 │ 215.03 ms │ 214.55 ms │ no change │
│ QQuery 8 │ 162.08 ms │ 160.66 ms │ no change │
│ QQuery 9 │ 224.89 ms │ 220.31 ms │ no change │
│ QQuery 10 │ 182.61 ms │ 182.01 ms │ no change │
│ QQuery 11 │ 73.35 ms │ 73.84 ms │ no change │
│ QQuery 12 │ 118.78 ms │ 113.55 ms │ no change │
│ QQuery 13 │ 221.80 ms │ 216.19 ms │ no change │
│ QQuery 14 │ 94.01 ms │ 91.71 ms │ no change │
│ QQuery 15 │ 120.00 ms │ 118.32 ms │ no change │
│ QQuery 16 │ 56.61 ms │ 55.85 ms │ no change │
│ QQuery 17 │ 272.92 ms │ 272.03 ms │ no change │
│ QQuery 18 │ 357.78 ms │ 316.55 ms │ +1.13x faster │
│ QQuery 19 │ 138.06 ms │ 134.08 ms │ no change │
│ QQuery 20 │ 127.96 ms │ 123.23 ms │ no change │
│ QQuery 21 │ 259.58 ms │ 260.55 ms │ no change │
│ QQuery 22 │ 41.91 ms │ 41.99 ms │ no change │
└──────────────┴───────────┴──────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (HEAD) │ 3401.11ms │
│ Total Time (opt_htlExpr_eval) │ 3324.65ms │
│ Average Time (HEAD) │ 154.60ms │
│ Average Time (opt_htlExpr_eval) │ 151.12ms │
│ Queries Faster │ 1 │
│ Queries Slower │ 0 │
│ Queries with No Change │ 21 │
│ Queries with Failure │ 0 │
└─────────────────────────────────┴───────────┘
```
</p>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2658958984
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -343,16 +343,8 @@ impl PhysicalExpr for HashTableLookupExpr {
// Check each hash against the hash table
let mut buf = MutableBuffer::from_len_zeroed(bit_util::ceil(num_rows, 8));
- for (idx, hash_value) in hash_array.values().iter().enumerate() {
- // Use get_matched_indices to check - if it returns any indices, the hash exists
- let (matched_indices, _) = self
- .hash_map
- .get_matched_indices(Box::new(std::iter::once((idx, hash_value))), None);
-
- if !matched_indices.is_empty() {
- bit_util::set_bit(buf.as_slice_mut(), idx);
- }
- }
+ self.hash_map
+ .set_bits_if_exists(hash_array.values(), buf.as_slice_mut());
Review Comment:
Done. Reusing the hashes buffer now. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#issuecomment-3707120691
> Very nice! I added 2 suggestions which could maybe improve it even further.
Thanks for reviewing! I've applied both suggestions. Could you please take another look? @Dandandan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2658990690
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -327,12 +329,24 @@ impl PhysicalExpr for HashTableLookupExpr {
Ok(false)
}
- fn evaluate(
- &self,
- batch: &arrow::record_batch::RecordBatch,
- ) -> Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let num_rows = batch.num_rows();
+ // 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>() {
Review Comment:
Isn't this always the case? We can remove the hashexpr while it is constructed to simplify the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #19602:
URL: https://github.com/apache/datafusion/pull/19602#discussion_r2658993049
##########
datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:
##########
@@ -343,16 +343,8 @@ impl PhysicalExpr for HashTableLookupExpr {
// Check each hash against the hash table
let mut buf = MutableBuffer::from_len_zeroed(bit_util::ceil(num_rows, 8));
- for (idx, hash_value) in hash_array.values().iter().enumerate() {
- // Use get_matched_indices to check - if it returns any indices, the hash exists
- let (matched_indices, _) = self
- .hash_map
- .get_matched_indices(Box::new(std::iter::once((idx, hash_value))), None);
-
- if !matched_indices.is_empty() {
- bit_util::set_bit(buf.as_slice_mut(), idx);
- }
- }
+ self.hash_map
Review Comment:
Looks great!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimize `HashTableLookupExpr::evaluate` [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan merged PR #19602:
URL: https://github.com/apache/datafusion/pull/19602
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org