[GitHub] [arrow-datafusion] mustafasrepo commented on pull request #6501: Support ordering analysis with expressions (not just columns) by Replace `OrderedColumn` with `PhysicalSortExpr`

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

mustafasrepo commented on PR #6501:
URL: https://github.com/apache/arrow-datafusion/pull/6501#issuecomment-1574428990

   > I think this PR looks great -- thank you @mustafasrepo and adds a neat feature. cc @mingmwang in case you have any interest in reviewing this
   > 
   > > However, because PhysicalSortExpr doesn't implement Hash trait (there is no trivial way to support this trait if any). We changed the EquivalentClass implementation so that it doesn't require Hash trait anymore.
   > 
   > We hit something similar when trying to make `LogicalPlan` implement hash (because of the `LogicalPlan::Extension` variant that has a `Arc<dyn UserDefinedLogicalNode>`
   > 
   > The solution we came up with was
   > 
   > https://docs.rs/datafusion-expr/25.0.0/datafusion_expr/logical_plan/trait.UserDefinedLogicalNode.html#tymethod.dyn_hash
   > 
   > And then implemented it like this: https://docs.rs/datafusion-expr/25.0.0/src/datafusion_expr/logical_plan/extension.rs.html#235-285
   
   I will experiment with using `dyn hash`, I think this will simplify the structure.


-- 
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