findepi commented on PR #16625: URL: https://github.com/apache/datafusion/pull/16625#issuecomment-3019616554
thanks for your review @ozankabak > * `FIRST_VALUE` and `LAST_VALUE` implementations use `requirement_satisfied` and `with_requirement_satisfied` names for this thing, IMO it would be a good idea to follow suit here for consistency/readability purposes. will align for array_agg i am ll for consistency. do you think we could update `AggregateUDFImpl::with_beneficial_ordering(beneficial_ordering bool)` signature to follow the desired naming? > * There are some changes where the planner was able to get away with appending a key to an already-existing sort, and operate the accumulator in an efficient mode. This PR loses that. Yes, i noticed the PR drops some redundant sorts, which is likely quite a big improvement for aggregations with GROUP BY. Am i reading this correctly? I agree that in some rare situations those sorts are still the optimal way to go. However, I don't see a way for UDAF to declare `OrderingRequirements::Soft`. Is this a hard blocker for this PR, or can we avoid scope creep? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
