Repository navigation
Move filter for HAVING and QUALIFY when volatile aliases are used above projection - #26850
Merged
zhuqi-lucas merged 6 commits intoOct 31, 2026
Merged
zhuqi-lucas merged 6 commits into
zhuqi-lucas merged 6 commits into
Conversation
matthewmturner
changed the title
HAVING and QUALIFY when volatile aliases are used
matthewmturner
changed the title
HAVING and QUALIFY when volatile aliases are usedHAVING and QUALIFY when volatile aliases are used above projection
matthewmturner
marked this pull request as ready for review
October 30, 2026 03:59
Contributor
Author
|
@xudong963 @zhuqi-lucas if you get the chance to review |
zhuqi-lucas
approved these changes
Oct 30, 2026
zhuqi-lucas
left a comment
zhuqi-lucas
left a comment
Contributor
There was a problem hiding this comment.
Thanks @matthewmturner, nice fix — the post-projection filter plus the physical pushdown guard both look right to me, and the resulting plan matches the subquery rewrite suggested in #7976. Possible follow-up: split conjuncts at planning time so HAVING <volatile alias> AND <aggregate> can still plan.
Contributor
Author
|
@zhuqi-lucas ill check how big of a change it would be to add split conjucts on this pr |
alamb
approved these changes
Oct 30, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you @matthewmturner and @zhuqi-lucas
| 05)--------TableScan: users projection=[id] | ||
| physical_plan | ||
| 01)FilterExec: r@2 > 0.5 | ||
| 02)--ProjectionExec: expr=[id@0 as id, row_number() ORDER BY [users.id ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@1 as rn, random() as r] |
Contributor
There was a problem hiding this comment.
good to see random still here
| ---- | ||
| 3 | ||
|
|
||
| # an aggregate that is not in the SELECT list cannot be combined with a |
Contributor
|
Thanks @matthewmturner and @alamb , LGTM, merged now. |
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
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?
Closes Incorrect result when use volatile function alias in having clause #7976
Commit 158a1b8 -> Makes test that fails, as expected, on the expected correct behavior
Commit 49e5f9c -> Dont push down volatile filter expressions - new tests still fail as we arent rewriting to subquery
Commit 3895261 -> Move the filter of volatile expression above projection
Commit 2f358c0 -> Split conjuncts so we can plan more cases following @zhuqi-lucas feedback.
Rationale for this change
The current behavior returns unexpected / incorrect results since the volatile expression used for evaluating predicate is different from the one used for projection.
What changes are included in this PR?
When a filter on a volatile alias is used with
HAVING/QUALIFYit is moved above the projection so it operates on the same value that is projected.What is the testing strategy for this PR?
SLT tests are added for the triggering cases and for cases that arent allowed with the new plan (primarily cases when there are filters on fields that arent in the projection so they are unavailable for filtering above the projection).
Are there any user-facing changes?