alamb commented on code in PR #17438:
URL: https://github.com/apache/datafusion/pull/17438#discussion_r2325757148
##########
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:
I think `self == other` will (re) compare all entries in `self.values` and
`other.values` again
we can probably avoid checking `values == other.values` here by just
checking the missing field
```suggestion
.filter(|cmp| *cmp != Ordering::Equal || self.schema ==
other.schema)
```
##########
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:
The same comment applies ot the other comparisons in this PR as well
--
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
