fix: Support aggregate expressions in QUALIFY#17313
Conversation
|
@Vedin can you please help review this PR? I see you are starting to do so on the original ticket #17210 (comment) |
|
In general, logic looks correct. The only unsupported thing is what I mentioned under the issue #17210 (comment) |
| Ok((plan, select_exprs_post_aggr, having_expr_post_aggr)) | ||
| // Rewrite the QUALIFY expression to use the columns produced by the | ||
| // aggregation. | ||
| let qualify_expr_post_aggr = if let Some(qualify_expr) = qualify_expr_opt { |
There was a problem hiding this comment.
nit: this logic is pretty similar to what is used for HAVING. What about creating a helper function for this helper logic?
There was a problem hiding this comment.
Definitely similar, although currently we still require multiple distinct function calls to check_column_satisfy_expr because for each expression (SELECT, HAVING, QUALIFY), we pass in diagnostic information that indicates which clause the error occurs in (CheckColumnsSatisfyExprsPurpose).
Given this, I'm not quite sure a helper function would help readability/redundancy too much
There was a problem hiding this comment.
Perhaps we can try it in a follow on PR
|
Hi @alamb friendly bump here. I think this should be good to merge and then we can tackle the second issue in another PR |
| Ok((plan, select_exprs_post_aggr, having_expr_post_aggr)) | ||
| // Rewrite the QUALIFY expression to use the columns produced by the | ||
| // aggregation. | ||
| let qualify_expr_post_aggr = if let Some(qualify_expr) = qualify_expr_opt { |
There was a problem hiding this comment.
Perhaps we can try it in a follow on PR
| let diag = do_query(query); | ||
| assert_snapshot!(diag.message, @"'person.first_name' must appear in GROUP BY clause because it's not an aggregate expression"); | ||
| assert_eq!(diag.span, Some(spans["a"])); | ||
| assert_snapshot!(diag.helps[0].message, @"Either add 'person.first_name' to GROUP BY clause, or use an aggregare function like ANY_VALUE(person.first_name)"); |
|
Thanks again @rkrishn7 |
3 participants
Which issue does this PR close?
QUALIFY#17210.What changes are included in this PR?
QUALIFYand rewrites any aggregates in theQUALIFYclause to use column references (using the expression's schema name).Are these changes tested?
Yes
Are there any user-facing changes?
No additional features, just fixes.