Perf: Short circuit expensive comparissions in GroupColumn - #23344
Closed
Rich-T-kid wants to merge 6 commits into
Closed
Perf: Short circuit expensive comparissions in GroupColumn #23344Rich-T-kid wants to merge 6 commits into
Rich-T-kid wants to merge 6 commits into
Conversation
Rich-T-kid
changed the title
Contributor
Author
|
cc @zhuqi-lucas as this is related to #22715 |
RatulDawar
reviewed
Jul 6, 2026
| /// Used to order columns in [`GroupValuesColumn::compare_order`] so cheap comparisons | ||
| /// eliminate rows before expensive ones are evaluated. | ||
| /// see <https://github.com/apache/datafusion/issues/23342> | ||
| fn compare_tier(data_type: &DataType) -> u8 { |
Contributor
There was a problem hiding this comment.
Shouldn't this belong to a common place as this can be used across other operators like HashJoins
Contributor
Author
There was a problem hiding this comment.
I suppose 🤔 , is there a specific case where you think this would be useful for hashJoins?
Contributor
There was a problem hiding this comment.
When JoinKeyComparator finds groups matching groups per row similarly, correct me if I am wrong. Here the compare method can also use this optimized ordering right ?
Contributor
Author
There was a problem hiding this comment.
Im honestly not too familiar with the join operator logic. but presumably the same concepts should apply there. Ideally i'd like to move forward with #23342. but I can make a follow up ticket to move this into a higher modules for code re-use
Rich-T-kid
changed the title
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (bf08818) to 0365d3c (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (bf08818) to 0365d3c (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (bf08818) to 0365d3c (merge-base) diff using: clickbench_partitioned 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
Author
|
😞 doesn't seem to have made an impact |
Contributor
It may due to this path is not the hot path for the benchmark. |
Contributor
Author
@zhuqi-lucas what do you mean? |
Rich-T-kid
force-pushed
the
rich-T-kid/short-circuit-groupColumn
branch
from
July 9, 2026 21:27
3d7524f to
de9430a
Compare
Contributor
Author
|
Now that Perf: Add short circuit for primitive vectorized equal_to has been merged, we should re-run the benchmarks for this PR. #23343 introduces early termination for PrimitiveGroupColumn, which accounts for 21 of the 28 GroupColumn trait implementations. |
Contributor
Author
@zhuqi-lucas friendly ping |
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (722b0dd) to f151c10 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (722b0dd) to f151c10 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (722b0dd) to f151c10 (merge-base) diff using: clickbench_partitioned 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23344 +/- ##
==========================================
- Coverage 80.71% 80.71% -0.01%
==========================================
Files 1090 1090
Lines 370339 370418 +79
Branches 370339 370418 +79
==========================================
+ Hits 298926 298978 +52
- Misses 53603 53616 +13
- Partials 17810 17824 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zhuqi-lucas
reviewed
Jul 24, 2026
zhuqi-lucas
left a comment
zhuqi-lucas
left a comment
Contributor
There was a problem hiding this comment.
Nice direction. Correctness OK — traced all 4 vectorized_equal_to impls, all short-circuit via AND-semantics so order-invariant for correctness.
Locally applied Rich's diff + the fixes below and ran cargo test -p datafusion-physical-plan aggregates::group_values: 33/33 pass.
Main finding: 3 more schema-order short-circuit loops live in scalarized_intern and scalarized_equal_to_remaining — both are hit by streaming (STREAMING = true) AND by the non-streaming fallback (scalarized_intern_remaining, called at line 495 from vectorized_intern). These lines aren't in the diff so I can't attach one-click suggestions, but the fix pattern is a direct copy of what you did in vectorized_equal_to_all:
mod.rs:400 (inside scalarized_intern) — replace the for (i, group_val) in self.group_values.iter().enumerate() block with:
for &i in &self.compare_order {
if !check_row_equal(
self.group_values[i].as_ref(),
group_idx_view.value() as usize,
&cols[i],
row,
) {
return false;
}
}mod.rs:861 (in scalarized_equal_to_remaining, non-inlined branch):
for &i in &self.compare_order {
if !check_row_equal(
self.group_values[i].as_ref(),
group_idx,
&cols[i],
row,
) {
check_result = false;
break;
}
}mod.rs:878 (in scalarized_equal_to_remaining, inlined branch):
for &i in &self.compare_order {
if !check_row_equal(
self.group_values[i].as_ref(),
group_idx,
&cols[i],
row,
) {
return false;
}
}Also please add a test (compiled + passed locally on your branch):
#[test]
fn compare_order_puts_cheap_columns_first() {
let schema = Arc::new(Schema::new(vec![
Field::new("s", DataType::Utf8, false),
Field::new("i", DataType::Int64, false),
Field::new("v", DataType::Utf8View, false),
Field::new("b", DataType::Boolean, false),
]));
let order = GroupValuesColumn::<false>::build_group_compare_order(&schema);
let pos = |i: usize| order.iter().position(|&x| x == i).unwrap();
assert!(pos(1) < pos(0), "int should come before utf8");
assert!(pos(3) < pos(2), "bool should come before utf8view");
}Goes into the existing #[cfg(test)] mod tests at mod.rs:1306.
| /// Used to order columns in [`GroupValuesColumn::compare_order`] so cheap comparisons | ||
| /// eliminate rows before expensive ones are evaluated. | ||
| /// see <https://github.com/apache/datafusion/issues/23342> | ||
| fn compare_tier(data_type: &DataType) -> u8 { |
Contributor
There was a problem hiding this comment.
- PR body says tiers
1 → 5but this returns1 / 2 / 3— please align. _ => 3is unreachable:try_newcallsbuild_group_columns?beforebuild_group_compare_order, andmake_group_columnrejects any type outside tier 1 / tier 2 upstream. Worth a one-line comment saying so — today it reads like the branch might fire.
Rich-T-kid
force-pushed
the
rich-T-kid/short-circuit-groupColumn
branch
from
July 24, 2026 15:08
722b0dd to
59d2cf9
Compare
Contributor
Author
|
@zhuqi-lucas thank you the review & catching other places where this optimization could be applied. 59d2cf9 should address all of your comments, could we try running the benchmarks again? |
Contributor
Author
|
CI is breaking for unrelated reason |
Contributor
Author
|
@zhuqi-lucas friendly ping |
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (84e87ce) to 3f81613 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (84e87ce) to 3f81613 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/short-circuit-groupColumn (84e87ce) to 3f81613 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing rich-T-kid/short-circuit-groupColumn (84e87ce) to 3f81613 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing rich-T-kid/short-circuit-groupColumn (84e87ce) to 3f81613 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing rich-T-kid/short-circuit-groupColumn (84e87ce) to 3f81613 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
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
5 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?
Rationale for this change
see #23342
Note : the exact ranking here is a bit arbitrary & i'm happy to change it. The core idea is that its much cheaper to do boolean checks and fixed size byte comparisons as opposed to chasing pointers to comparing strings.
an alternative approach may be to split this into two tiers:
What changes are included in this PR?
compare_tier()to rank each supported DataType by comparison cost (1 = cheap fixed-width integers → 3 = expensive variable-length bytes)compare_order: Vec to GroupValuesColumn, built once at construction viabuild_group_compare_order(), sovectorized_equal_toevaluates columns cheapest-first and maximizes early-exit savings before reaching expensive string comparisonsAre these changes tested?
This PR only change the order in which comparisons occurs. functionally nothing changes.
Are there any user-facing changes?
no