From 43c0cf2e53fded0157e1bfb547d63898f004ca12 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 5 Sep 2025 11:06:46 +0200 Subject: [PATCH 1/3] Fix `PartialOrd` for DDL & DML Before the changes, `PartialOrd` could return `Some(Equal)` for two values that are not equal in `PartialEq` sense. This is violation of `PartialOrd` contract. The fix is to consult eq inside ord implementation. If ord thinks two instances are equal, but they are not equal in Eq sense, they are considered incomparable. --- datafusion/expr/src/logical_plan/ddl.rs | 18 +++++++++++++++--- datafusion/expr/src/logical_plan/dml.rs | 2 ++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/datafusion/expr/src/logical_plan/ddl.rs b/datafusion/expr/src/logical_plan/ddl.rs index 827e2812ecae..aa5a1feb9d37 100644 --- a/datafusion/expr/src/logical_plan/ddl.rs +++ b/datafusion/expr/src/logical_plan/ddl.rs @@ -292,7 +292,9 @@ impl PartialOrd for CreateExternalTable { unbounded: &other.unbounded, constraints: &other.constraints, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -348,6 +350,7 @@ impl PartialOrd for CreateCatalog { Some(Ordering::Equal) => self.if_not_exists.partial_cmp(&other.if_not_exists), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -369,6 +372,7 @@ impl PartialOrd for CreateCatalogSchema { Some(Ordering::Equal) => self.if_not_exists.partial_cmp(&other.if_not_exists), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -390,6 +394,7 @@ impl PartialOrd for DropTable { Some(Ordering::Equal) => self.if_exists.partial_cmp(&other.if_exists), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -411,6 +416,7 @@ impl PartialOrd for DropView { Some(Ordering::Equal) => self.if_exists.partial_cmp(&other.if_exists), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -437,6 +443,7 @@ impl PartialOrd for DropCatalogSchema { }, cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -486,7 +493,9 @@ impl PartialOrd for CreateFunction { return_type: &other.return_type, params: &other.params, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -566,6 +575,7 @@ impl PartialOrd for DropFunction { Some(Ordering::Equal) => self.if_exists.partial_cmp(&other.if_exists), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -608,7 +618,9 @@ impl PartialOrd for CreateIndex { unique: &other.unique, if_not_exists: &other.if_not_exists, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/logical_plan/dml.rs b/datafusion/expr/src/logical_plan/dml.rs index 369b91e204b9..b380929c1517 100644 --- a/datafusion/expr/src/logical_plan/dml.rs +++ b/datafusion/expr/src/logical_plan/dml.rs @@ -81,6 +81,7 @@ impl PartialOrd for CopyTo { }, cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -217,6 +218,7 @@ impl PartialOrd for DmlStatement { }, cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } From c6252f4e1b725c72e21f5755b2822a900c64d006 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 5 Sep 2025 11:21:27 +0200 Subject: [PATCH 2/3] Fix `PartialOrd` for logical plan nodes and expressions Before the changes, `PartialOrd` could return `Some(Equal)` for two values that are not equal in `PartialEq` sense. This is violation of `PartialOrd` contract. The fix is to consult eq inside ord implementation. If ord thinks two instances are equal, but they are not equal in Eq sense, they are considered incomparable. --- datafusion/expr/src/expr.rs | 4 ++- datafusion/expr/src/logical_plan/extension.rs | 7 ++-- datafusion/expr/src/logical_plan/plan.rs | 36 ++++++++++++++----- datafusion/expr/src/udaf.rs | 5 +-- datafusion/expr/src/udwf.rs | 2 +- .../optimizer/src/optimize_projections/mod.rs | 2 ++ datafusion/optimizer/src/push_down_filter.rs | 4 ++- datafusion/optimizer/src/push_down_limit.rs | 8 +++-- .../tests/cases/roundtrip_logical_plan.rs | 1 + 9 files changed, 50 insertions(+), 19 deletions(-) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index 0eef8a00a915..8979f5483250 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -748,7 +748,9 @@ impl PartialOrd for Alias { let Some(Ordering::Equal) = cmp else { return cmp; }; - self.name.partial_cmp(&other.name) + self.name + .partial_cmp(&other.name) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/logical_plan/extension.rs b/datafusion/expr/src/logical_plan/extension.rs index e10b74098b68..a8ee7885644a 100644 --- a/datafusion/expr/src/logical_plan/extension.rs +++ b/datafusion/expr/src/logical_plan/extension.rs @@ -150,7 +150,7 @@ pub trait UserDefinedLogicalNode: fmt::Debug + Send + Sync { /// directly because it must remain object safe. fn dyn_hash(&self, state: &mut dyn Hasher); - /// Compare `other`, respecting requirements from [std::cmp::Eq]. + /// Compare `other`, respecting requirements from [Eq]. /// /// Note: consider using [`UserDefinedLogicalNodeCore`] instead of /// [`UserDefinedLogicalNode`] directly. @@ -188,6 +188,9 @@ pub trait UserDefinedLogicalNode: fmt::Debug + Send + Sync { /// Note: [`UserDefinedLogicalNode`] is not constrained by [`Eq`] /// directly because it must remain object safe. fn dyn_eq(&self, other: &dyn UserDefinedLogicalNode) -> bool; + + /// Compare `other`, respecting requirements from [PartialOrd]. + /// Must return `Some(Equal)` if and only if `self.dyn_eq(other)`. fn dyn_ord(&self, other: &dyn UserDefinedLogicalNode) -> Option; /// Returns `true` if a limit can be safely pushed down through this @@ -312,7 +315,7 @@ pub trait UserDefinedLogicalNodeCore: } /// Automatically derive UserDefinedLogicalNode to `UserDefinedLogicalNode` -/// to avoid boiler plate for implementing `as_any`, `Hash` and `PartialEq` +/// to avoid boiler plate for implementing `as_any`, `Hash`, `PartialEq` and `PartialOrd`. impl UserDefinedLogicalNode for T { fn as_any(&self) -> &dyn Any { self diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index a354fb666f99..e96cfeb05c02 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -2060,7 +2060,9 @@ pub struct EmptyRelation { // Manual implementation needed because of `schema` field. Comparison excludes this field. impl PartialOrd for EmptyRelation { fn partial_cmp(&self, other: &Self) -> Option { - self.produce_one_row.partial_cmp(&other.produce_one_row) + self.produce_one_row + .partial_cmp(&other.produce_one_row) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -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 { - self.values.partial_cmp(&other.values) + self.values + .partial_cmp(&other.values) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2139,6 +2143,7 @@ impl PartialOrd for Projection { Some(Ordering::Equal) => self.input.partial_cmp(&other.input), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2249,6 +2254,7 @@ impl PartialOrd for SubqueryAlias { Some(Ordering::Equal) => self.alias.partial_cmp(&other.alias), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2592,7 +2598,9 @@ impl PartialOrd for TableScan { filters: &other.filters, fetch: &other.fetch, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2915,7 +2923,9 @@ impl Union { // Manual implementation needed because of `schema` field. Comparison excludes this field. impl PartialOrd for Union { fn partial_cmp(&self, other: &Self) -> Option { - self.inputs.partial_cmp(&other.inputs) + self.inputs + .partial_cmp(&other.inputs) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3196,7 +3206,9 @@ impl PartialOrd for Explain { stringified_plans: &other.stringified_plans, logical_optimization_succeeded: &other.logical_optimization_succeeded, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3219,6 +3231,7 @@ impl PartialOrd for Analyze { Some(Ordering::Equal) => self.input.partial_cmp(&other.input), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3446,7 +3459,9 @@ impl PartialOrd for DistinctOn { sort_expr: &other.sort_expr, input: &other.input, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3633,6 +3648,7 @@ impl PartialOrd for Aggregate { } cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3905,7 +3921,9 @@ impl PartialOrd for Join { join_constraint: &other.join_constraint, null_equality: &other.null_equality, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -4070,7 +4088,9 @@ impl PartialOrd for Unnest { dependency_indices: &other.dependency_indices, options: &other.options, }; - comparable_self.partial_cmp(&comparable_other) + comparable_self + .partial_cmp(&comparable_other) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index e1d36da9c4db..bcb57fef633b 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -978,16 +978,13 @@ impl PartialEq for dyn AggregateUDFImpl { } } -// TODO (https://github.com/apache/datafusion/issues/17064) PartialOrd is not consistent with PartialEq for `dyn AggregateUDFImpl` and it should be -// Manual implementation of `PartialOrd` -// There might be some wackiness with it, but this is based on the impl of eq for AggregateUDFImpl -// https://users.rust-lang.org/t/how-to-compare-two-trait-objects-for-equality/88063/5 impl PartialOrd for dyn AggregateUDFImpl { fn partial_cmp(&self, other: &Self) -> Option { match self.name().partial_cmp(other.name()) { Some(Ordering::Equal) => self.signature().partial_cmp(other.signature()), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/udwf.rs b/datafusion/expr/src/udwf.rs index 29bf379f60fb..b6afb894d8fd 100644 --- a/datafusion/expr/src/udwf.rs +++ b/datafusion/expr/src/udwf.rs @@ -434,13 +434,13 @@ impl PartialEq for dyn WindowUDFImpl { } } -// TODO (https://github.com/apache/datafusion/issues/17064) PartialOrd is not consistent with PartialEq for `dyn WindowUDFImpl` and it should be impl PartialOrd for dyn WindowUDFImpl { fn partial_cmp(&self, other: &Self) -> Option { match self.name().partial_cmp(other.name()) { Some(Ordering::Equal) => self.signature().partial_cmp(other.signature()), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/optimizer/src/optimize_projections/mod.rs b/datafusion/optimizer/src/optimize_projections/mod.rs index 97402c990b83..3d89ed5439fc 100644 --- a/datafusion/optimizer/src/optimize_projections/mod.rs +++ b/datafusion/optimizer/src/optimize_projections/mod.rs @@ -902,6 +902,7 @@ mod tests { Some(Ordering::Equal) => self.input.partial_cmp(&other.input), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -989,6 +990,7 @@ mod tests { } cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/optimizer/src/push_down_filter.rs b/datafusion/optimizer/src/push_down_filter.rs index 27c2499c8a26..99d216cbe608 100644 --- a/datafusion/optimizer/src/push_down_filter.rs +++ b/datafusion/optimizer/src/push_down_filter.rs @@ -1966,7 +1966,9 @@ mod tests { // Manual implementation needed because of `schema` field. Comparison excludes this field. impl PartialOrd for NoopPlan { fn partial_cmp(&self, other: &Self) -> Option { - self.input.partial_cmp(&other.input) + self.input + .partial_cmp(&other.input) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/optimizer/src/push_down_limit.rs b/datafusion/optimizer/src/push_down_limit.rs index ec042dd350ca..0198e2e7369d 100644 --- a/datafusion/optimizer/src/push_down_limit.rs +++ b/datafusion/optimizer/src/push_down_limit.rs @@ -312,7 +312,9 @@ mod test { // Manual implementation needed because of `schema` field. Comparison excludes this field. impl PartialOrd for NoopPlan { fn partial_cmp(&self, other: &Self) -> Option { - self.input.partial_cmp(&other.input) + self.input + .partial_cmp(&other.input) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -365,7 +367,9 @@ mod test { // Manual implementation needed because of `schema` field. Comparison excludes this field. impl PartialOrd for NoLimitNoopPlan { fn partial_cmp(&self, other: &Self) -> Option { - self.input.partial_cmp(&other.input) + self.input + .partial_cmp(&other.input) + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs index 606d32ee4dc9..877dcba5a82e 100644 --- a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs @@ -92,6 +92,7 @@ impl PartialOrd for MockUserDefinedLogicalPlan { Some(Ordering::Equal) => self.inputs.partial_cmp(&other.inputs), cmp => cmp, } + .filter(|cmp| *cmp != Ordering::Equal || self == other) } } From 2f0b04ea52c070239e73255212dd68ca11b476aa Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 9 Sep 2025 17:13:52 +0200 Subject: [PATCH 3/3] Link to issue --- datafusion/expr/src/expr.rs | 1 + datafusion/expr/src/logical_plan/ddl.rs | 9 +++++++++ datafusion/expr/src/logical_plan/dml.rs | 2 ++ datafusion/expr/src/logical_plan/plan.rs | 12 ++++++++++++ datafusion/expr/src/udaf.rs | 1 + datafusion/expr/src/udwf.rs | 1 + datafusion/optimizer/src/optimize_projections/mod.rs | 2 ++ datafusion/optimizer/src/push_down_filter.rs | 1 + datafusion/optimizer/src/push_down_limit.rs | 2 ++ .../substrait/tests/cases/roundtrip_logical_plan.rs | 1 + 10 files changed, 32 insertions(+) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index 8979f5483250..75b0d2eb4416 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -750,6 +750,7 @@ impl PartialOrd for Alias { }; self.name .partial_cmp(&other.name) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/logical_plan/ddl.rs b/datafusion/expr/src/logical_plan/ddl.rs index aa5a1feb9d37..d477ca1ad0d5 100644 --- a/datafusion/expr/src/logical_plan/ddl.rs +++ b/datafusion/expr/src/logical_plan/ddl.rs @@ -294,6 +294,7 @@ impl PartialOrd for CreateExternalTable { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -350,6 +351,7 @@ impl PartialOrd for CreateCatalog { Some(Ordering::Equal) => self.if_not_exists.partial_cmp(&other.if_not_exists), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -372,6 +374,7 @@ impl PartialOrd for CreateCatalogSchema { Some(Ordering::Equal) => self.if_not_exists.partial_cmp(&other.if_not_exists), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -394,6 +397,7 @@ impl PartialOrd for DropTable { Some(Ordering::Equal) => self.if_exists.partial_cmp(&other.if_exists), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -416,6 +420,7 @@ impl PartialOrd for DropView { Some(Ordering::Equal) => self.if_exists.partial_cmp(&other.if_exists), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -443,6 +448,7 @@ impl PartialOrd for DropCatalogSchema { }, cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -495,6 +501,7 @@ impl PartialOrd for CreateFunction { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -575,6 +582,7 @@ impl PartialOrd for DropFunction { Some(Ordering::Equal) => self.if_exists.partial_cmp(&other.if_exists), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -620,6 +628,7 @@ impl PartialOrd for CreateIndex { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/logical_plan/dml.rs b/datafusion/expr/src/logical_plan/dml.rs index b380929c1517..b8448a5da6c4 100644 --- a/datafusion/expr/src/logical_plan/dml.rs +++ b/datafusion/expr/src/logical_plan/dml.rs @@ -81,6 +81,7 @@ impl PartialOrd for CopyTo { }, cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -218,6 +219,7 @@ impl PartialOrd for DmlStatement { }, cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index e96cfeb05c02..902d2f22e6fa 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -2062,6 +2062,7 @@ impl PartialOrd for EmptyRelation { fn partial_cmp(&self, other: &Self) -> Option { self.produce_one_row .partial_cmp(&other.produce_one_row) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2118,6 +2119,7 @@ impl PartialOrd for Values { fn partial_cmp(&self, other: &Self) -> Option { self.values .partial_cmp(&other.values) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2143,6 +2145,7 @@ impl PartialOrd for Projection { Some(Ordering::Equal) => self.input.partial_cmp(&other.input), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2254,6 +2257,7 @@ impl PartialOrd for SubqueryAlias { Some(Ordering::Equal) => self.alias.partial_cmp(&other.alias), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2600,6 +2604,7 @@ impl PartialOrd for TableScan { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -2925,6 +2930,7 @@ impl PartialOrd for Union { fn partial_cmp(&self, other: &Self) -> Option { self.inputs .partial_cmp(&other.inputs) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3208,6 +3214,7 @@ impl PartialOrd for Explain { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3231,6 +3238,7 @@ impl PartialOrd for Analyze { Some(Ordering::Equal) => self.input.partial_cmp(&other.input), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3461,6 +3469,7 @@ impl PartialOrd for DistinctOn { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3648,6 +3657,7 @@ impl PartialOrd for Aggregate { } cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -3923,6 +3933,7 @@ impl PartialOrd for Join { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -4090,6 +4101,7 @@ impl PartialOrd for Unnest { }; comparable_self .partial_cmp(&comparable_other) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index bcb57fef633b..dc3f7bddc40a 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -984,6 +984,7 @@ impl PartialOrd for dyn AggregateUDFImpl { Some(Ordering::Equal) => self.signature().partial_cmp(other.signature()), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/expr/src/udwf.rs b/datafusion/expr/src/udwf.rs index b6afb894d8fd..a7b4302dd893 100644 --- a/datafusion/expr/src/udwf.rs +++ b/datafusion/expr/src/udwf.rs @@ -440,6 +440,7 @@ impl PartialOrd for dyn WindowUDFImpl { Some(Ordering::Equal) => self.signature().partial_cmp(other.signature()), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/optimizer/src/optimize_projections/mod.rs b/datafusion/optimizer/src/optimize_projections/mod.rs index 3d89ed5439fc..1e9c40a5d781 100644 --- a/datafusion/optimizer/src/optimize_projections/mod.rs +++ b/datafusion/optimizer/src/optimize_projections/mod.rs @@ -902,6 +902,7 @@ mod tests { Some(Ordering::Equal) => self.input.partial_cmp(&other.input), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -990,6 +991,7 @@ mod tests { } cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/optimizer/src/push_down_filter.rs b/datafusion/optimizer/src/push_down_filter.rs index 99d216cbe608..5a3d57d65afc 100644 --- a/datafusion/optimizer/src/push_down_filter.rs +++ b/datafusion/optimizer/src/push_down_filter.rs @@ -1968,6 +1968,7 @@ mod tests { fn partial_cmp(&self, other: &Self) -> Option { self.input .partial_cmp(&other.input) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/optimizer/src/push_down_limit.rs b/datafusion/optimizer/src/push_down_limit.rs index 0198e2e7369d..c5a2e6578805 100644 --- a/datafusion/optimizer/src/push_down_limit.rs +++ b/datafusion/optimizer/src/push_down_limit.rs @@ -314,6 +314,7 @@ mod test { fn partial_cmp(&self, other: &Self) -> Option { self.input .partial_cmp(&other.input) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } @@ -369,6 +370,7 @@ mod test { fn partial_cmp(&self, other: &Self) -> Option { self.input .partial_cmp(&other.input) + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } } diff --git a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs index 877dcba5a82e..616dc917efe4 100644 --- a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs @@ -92,6 +92,7 @@ impl PartialOrd for MockUserDefinedLogicalPlan { Some(Ordering::Equal) => self.inputs.partial_cmp(&other.inputs), cmp => cmp, } + // TODO (https://github.com/apache/datafusion/issues/17477) avoid recomparing all fields .filter(|cmp| *cmp != Ordering::Equal || self == other) } }