Repository navigation
fix: evaluate grouped aggregate arguments after FILTER - #24444
Conversation
github-actions
Bot
added
sqllogictest
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24444 +/- ##
==========================================
+ Coverage 81.23% 81.85% +0.61%
==========================================
Files 1112 1117 +5
Lines 390635 405710 +15075
Branches 390635 405710 +15075
==========================================
+ Hits 317350 332080 +14730
- Misses 54650 55180 +530
+ Partials 18635 18450 -185 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2010YOUY01
left a comment
2010YOUY01
left a comment
There was a problem hiding this comment.
Thank you. I've verified that this is consistent with PostgreSQL's behavior, and the implementation looks good to me. I just left a few suggestions for simplification.
I haven't reviewed the legacy implementation changes in grouped_hash_stream.rs yet. That part is harder for me to reason about—could we revert those changes for now? I think that's acceptable since we should be able to deprecate the legacy implementation in the next release. The downside would be a temporary behavior gap, but I think that's probably fine.
One thought for further improvement: it seems more efficient to handle the aggregate filter entirely during preprocessing. For example, we could first evaluate the filter, then use the resulting selection for late materialization of the group keys and aggregate arguments.
I think we currently still pass the filter through because the existing GroupsAccumulator API is able to handle the filter, but that may no longer be necessary once filtering has already been applied upstream.
|
@2010YOUY01 Thanks for the review! I've updated the PR based on your comments:
|
This makes sense. It looks like the non-grouped Accumulator path already filters the batch before evaluating its arguments. For grouped aggregation, I think we would still need to create groups from all rows, then pass only the selected argument values and their corresponding group indices to GroupsAccumulator. That should allow us to avoid passing the filter through. I’ll take a closer look at this approach. |
|
re-run CI |
Which issue does this PR close?
Rationale for this change
Grouped aggregates evaluate their arguments before applying
FILTER, which can cause errors on rows that should not participate in the aggregate.What changes are included in this PR?
Evaluate aggregate filters first and use the resulting filters as selections when evaluating arguments.
This covers migrated hash and ordered aggregation, legacy grouped hash aggregation, and both skip-partial paths.
Are these changes tested?
Yes. slts and unit tests cover migrated, legacy, ordered, and skip-partial execution.
Are there any user-facing changes?
No public APIs are changed. Grouped aggregate arguments are no longer evaluated for rows rejected by
FILTER.