findepi commented on code in PR #17438:
URL: https://github.com/apache/datafusion/pull/17438#discussion_r2330190630
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2114,7 +2116,9 @@ pub struct Values {
// Manual implementation needed because of `schema` field. Comparison excludes
this field.
impl PartialOrd for Values {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
- self.values.partial_cmp(&other.values)
+ self.values
+ .partial_cmp(&other.values)
+ .filter(|cmp| *cmp != Ordering::Equal || self == other)
Review Comment:
You're right, but the proposed code is not self-maintainable. I.e. the code
likely won't be updated when (a) a new field gets added and (b) neither
compiler nor any tests complains.
For this to work, we would need a bigger change for the Ord implementation
where we destructure the compared object, which then forces code update when
any new field is added.
I'd propose this more elaborate change to be a follow-up, if you don't mind?
--
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]
