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
Implementing Filter Clause for aggregates #1309
Conversation
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.
Thanks for the PR! It looks good, except indeed I think the binding process is a bit overcomplicated. I have added some suggestions on how to simplify it.
src/execution/operator/aggregate/physical_perfecthash_aggregate.cpp
Outdated
Show resolved
Hide resolved
src/include/duckdb/execution/operator/aggregate/physical_hash_aggregate.hpp
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,273 @@ | |||
# name: test/sql/filter/test_filter_clause.test | |||
# description: Test aggregation with filter clause |
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 would like some more test cases:
- Query with many different filter clauses (e.g. 5 aggregates, 5 different filters)
- Filter with some more complex aggregates:
COVAR_POP
(multiple input columns),STRING_AGG
(strings) andARRAY_AGG
(lists) - DISTINCT aggregates
Also; looking at these tests I would not be surprised if all of them use the perfect hash aggregate. You can force the regular hash aggregate to be used by using very spaced out groups (e.g. [0, 10000000, 20000000, ....]
).
Thanks, this looks great now! |
So this |
auto &bound_ref_expr = (BoundReferenceExpression &)*aggr.filter; | ||
auto it = ht.find(aggr.filter.get()); | ||
if (it == ht.end()) { | ||
aggregate_input_chunk.data[aggregate_input_idx].Reference(input.data[bound_ref_expr.index]); |
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.
This thing
Will fix just thought I'd note for future reference |
This PR gives a (potentially over-complicated binding process, please check that) implementation of the filter clause for aggregates #896