Perf: Short circuit expensive comparissions in GroupColumn #23344
Open
Rich-T-kid wants to merge 3 commits into
Open
Perf: Short circuit expensive comparissions in GroupColumn #23344Rich-T-kid wants to merge 3 commits into
Rich-T-kid wants to merge 3 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? |
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
4 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 → 5 = 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