fix: preserve aggregate alias qualifiers in order-by rewrite #18767
Conversation
martin-g
left a comment
martin-g
left a comment
There was a problem hiding this comment.
Maybe add some new unit test(s).
| } | ||
|
|
||
| fn qualify_column(column: Column) -> Column { | ||
| if column.relation.is_some() { |
There was a problem hiding this comment.
| if column.relation.is_some() { | |
| if column.relation.is_some() || !column.name.contains('.') { |
| alias.expr = Box::new(ensure_column_qualifiers(*alias.expr)); | ||
| Expr::Alias(alias) | ||
| } | ||
| other => other, |
There was a problem hiding this comment.
What about expressions which contain nested columns ?
|
@martin-g Please check. Thanks! |
|
Thank you @kumarUjjawal 🙏 Can we please figure out why the CI was not failing ? It seems like we have a testing gap and even if this PR fixes the issues, the testing gap will allow it to potentially start failing again |
I will investigate further. |
|
@alamb My understanding is the |
| assert_eq!( | ||
| parsed.relation, | ||
| Some(TableReference::Bare { | ||
| table: "min(t".into() |
There was a problem hiding this comment.
Is min( expected here ?!
I'd expect just t
| table: "min(t".into() | ||
| }) | ||
| ); | ||
| assert_eq!(parsed.name, "c2)"); |
There was a problem hiding this comment.
And here I'd expect just c2 without the trailing )
There was a problem hiding this comment.
I’ve dropped the whole “split on dot” logic, so we no longer try to invent qualifiers like min(t / c2). The rewrite now just reuses whatever alias string the projection produced (e.g. the flat "min(t.c2)"), and the unit test asserts that exact name using a derived_col("min(t.c2)"). So the expectations you highlighted are gone.
There was a problem hiding this comment.
Let's see what others think about this.
For me the parsing is broken if min( appears in the table (reference) name and ) appears in the plain column name.
There was a problem hiding this comment.
I think you might have reviewed a earlier version of my code because I have reverted to simply reusing the projection’s alias string, so we no longer invent table references like min(t).
There was a problem hiding this comment.
Indeed! It seems I was talking about a previous state of this PR.
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
But how does this fit with CI succeeding and |
4 participants
Which issue does this PR close?
rewrite_sort_cols_by_agg_aliaswas rewritingORDER BY min(c2)to a column named justmin(t.c2)the rewritten sort expression didn’t match the schema entry so the assertion failed.rewrite_sort_cols_by_agg_aliastest failing when runningcargo testondatafusion-expr#18749Rationale for this change
What changes are included in this PR?
Are these changes tested?
Ran both the module level and project level tests.
Are there any user-facing changes?
No