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
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.