Remove physical sort parameters on aggregate window functions #12009
Conversation
|
@jayzhan211 Would you mind taking a look at this one? You recently worked on the impacted code and I could use a second opinion on this. |
|
I think it makes sense to have either window or aggregate to bring along ordering information. Although I'm not sure who should take cares of at the end. |
| .schema(Arc::new(input_schema.clone())) | ||
| .alias(name) | ||
| .order_by(order_by.to_vec()) | ||
| .sort_exprs(sort_exprs) |
There was a problem hiding this comment.
sort_exprs is removed. I think we could take a look again with the latest main branch
There was a problem hiding this comment.
Thank you. I rebased on main and ended up getting a different error, but I still think we need a correction. I think I'll write up a unit test as well.
There was a problem hiding this comment.
I guess after we introduce WindowExprBuilder, we could have a nice builder for createing window aggregate function with single order_by.
… handled by the window function
timsaucer
force-pushed
the
bugfix/order-on-window-aggregate-fns
branch
from
5eb0bea to
bcf6f5a
Compare
|
Thanks @timsaucer |
2 participants
Which issue does this PR close?
Closes #11981
Rationale for this change
When performing a window function using a UDAF, which all of our aggregate functions are, causes an error in the physical plan. Specifically it generates a
SchemaError::FieldNotFound. This appears to be because theDFSchemais empty in the minimal example showing in the bug report. However the ordering operation appears to already be handled by the window function call.What changes are included in this PR?
Removes the generation of the ordering and sorting, which are essentially duplicates of each other, and only leaves the ordering parameters on the window function definition.
Are these changes tested?
All unit tests working.
The minimum demonstration of the error in the bug report now works.
Tested against my project where I initially discovered the problem.
Are there any user-facing changes?
None.
Additional Context
I must admit I don't fully understand the rationale behind setting the ordering/sort parameters on both the window function definition and also within the aggregate function. So this code change does resolve my immediate issue, but I am not 100% confident it will not have unintended side effects.