Repository navigation
Fix HAS(...) == 1 breaking multi-level aggregations (#575) - #589
Merged
Merged
Conversation
knassre-bodo
force-pushed
the
Hadia/fix_has
branch
from
October 5, 2026 22:32
1fab2f1 to
1002470
Compare
hadia206
marked this pull request as ready for review
October 6, 2026 18:10
hadia206
requested review from
a team,
john-sanchez31,
juankx-bodo and
knassre-bodo
and removed request for
a team
October 6, 2026 18:10
hadia206
commented
Oct 6, 2026
| @@ -1,10 +1,10 @@ | |||
| ROOT(columns=[('name', anything_c_name), ('avg_diff', avg_day_diff)], orderings=[(avg_day_diff):desc_last], limit=5:numeric) | |||
| AGGREGATE(keys={'c_custkey': o_custkey}, aggregations={'anything_c_name': ANYTHING(c_name), 'avg_day_diff': AVG(day_diff)}) | |||
| PROJECT(columns={'c_name': c_name, 'day_diff': DATEDIFF('days':string, PREV(args=[o_orderdate], partition=[o_custkey], order=[(o_orderdate):asc_last]), o_orderdate), 'o_custkey': o_custkey}) | |||
Contributor
Author
There was a problem hiding this comment.
The bug fix allowed the optimization to be be correctly applied to these 2 queries, producing more efficient plans for them.
john-sanchez31
approved these changes
Oct 6, 2026
john-sanchez31
left a comment
john-sanchez31
left a comment
Contributor
There was a problem hiding this comment.
Looks good! Just couple of comments below
| "Customer Churn Risk / Recency": "The elapsed time in days between the current date and a customer's most recent invoice_date." | ||
| } | ||
| } | ||
| ] No newline at end of file |
| new_join_columns[join_name] = add_input_name(col_expr, agg_alias) | ||
| new_join_columns[join_name] = ColumnReference( | ||
| col_name, col_expr.data_type, agg_alias | ||
| ) |
Contributor
There was a problem hiding this comment.
Can we add a comment about this change?
Contributor
Author
There was a problem hiding this comment.
There's a comment already in the code, above the loop.
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
3 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.
Linked ticket
Closes #575
Type of change
What changed and why?
HAS(...) == 1broke multi-level aggregationson relationships marked"always matches": false, producing invalid SQL with a malformed, doubly-nestedSUM(SUM(...)).Root cause: an optimizer pass (
pull_aggregates_above_joins) assumed an aggregate's input was always raw data, but in this case it was actually an already-aggregated result so it got aggregated a second time.Fix: changed how that pass adds an aggregate's input into the new plan, it now references the input's output columns by name.
How I tested this?
Notes for reviewers