Re: [PR] Support ORDER BY in AggregateUDF [arrow-datafusion]

["jayzhan211 (via GitHub)" <gi...@apache.org>]

jayzhan211 commented on PR #9249:
URL: https://github.com/apache/arrow-datafusion/pull/9249#issuecomment-1972596074

   I think the problem here is that we need logical expr (ordering and schema) for accumulator. But the arguments pass into `create_aggregate_expr` are already physical expr. The reason why we need logical expr is that when we defining udaf, we only know logical expr but not physical expr.
   
   https://github.com/apache/arrow-datafusion/blob/10d5f2df1c8f81a0d64a8644cfa429331f1e8ac3/datafusion/physical-plan/src/udaf.rs#L40-L45
   
   I'm think about changing the function here to let us done the converting of logical to physical lately, so we can keep the logical expr for accumulator.
   
   Other builtin accumulator don't have the problem because they are all define in physical-expr unlike udaf.
   
   @alamb Does the move of physical expression conversion make senses?


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org