2010YOUY01 commented on PR #13897: URL: https://github.com/apache/datafusion/pull/13897#issuecomment-2561922401
One problem is it's not obvious why it works, though it's only a one-line function 🤔 It should work in common case but i'm not sure it's correct for every corner case (e.g. topK aggregate), given now how ordering will be preserved is not documented in `AggregateExec` If anyone is familiar with this part, it will be super helpful to add more comment to aggregate execution, and point to that from this PR's change, like ``` If input to AggregateExec is not ordered, no output order will be guaranteed If input is ordered by group keys, the streaming aggregation path will be used and output will maintain the same ordering ... ``` > > I recommend some tests if possible to avoid breakages during future refactorings > > Unless some downstream rules explicitly check the `maintains_input_order` flag of the ExecutionPlan (which we do in our fork), this implementation does not benefit to the current code. Maybe we can mark or comment something like 'experimental' to this method, if it's not tested? -- 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
