-
Notifications
You must be signed in to change notification settings - Fork 1.8k
perf: skip hash aggregation multi group by for already not found #19107
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
base: main
Are you sure you want to change the base?
Conversation
|
run benchmarks |
|
run benchmarks aggregate_query_sql |
|
🤖 Hi @rluvaton, thanks for the request (#19107 (comment)).
Please choose one or more of these with |
|
Show benchmark queue |
|
🤖 Hi @rluvaton, you asked to view the benchmark queue (#19107 (comment)).
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
@alamb, can you run |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
run benchmark aggregate_query_sql |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
This shows no improvements but currently, the high cardinality benchmarks are doing group by on 2 columns only so only the second column will be skipped, we need a lot more columns for it to be beneficial |
|
🤖 |
|
🤖: Benchmark completed Details
|
Which issue does this PR close?
N/A
Rationale for this change
make aggregation faster.
currently even if we reached that nothing equal to, we will continue iterating over the arrays
What changes are included in this PR?
track the number of equal to and when reach 0, break
Are these changes tested?
existing tests
Are there any user-facing changes?
nope, even though the group by impl struct are public they made public only for accessing in benchmarks