stuhood commented on code in PR #24766: URL: https://github.com/apache/datafusion/pull/24766#discussion_r4009816931
########## datafusion/physical-expr/src/partitioning.rs: ########## Review Comment: The fact that `PartialEq` is derived here is going to confuse callers: `enforce_distribution_relationships` uses equality to check whether two `RangePartitioning` instances are compatible. The `samples` should not actually be included in that equality check: only the `split_points`. There are two schools of thought on that, and I don't know which DataFusion prefers: * School 1: `PartialEq` should always represent structural equality, and so should basically always be derived. Other definitions of equality should be provided by other methods. * School 2: `PartialEq` should be the most useful definition of equality, even if that is not structural. I expect that @gene-bordegaray has an opinion on this one. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
