This is an automated email from the ASF dual-hosted git repository.
findepi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new 8494a3967a Derive `WindowUDFImpl` equality, hash from `Eq`, `Hash`
traits (#17081)
8494a3967a is described below
commit 8494a3967a413ef263a50b87ce8b5924eb1aa948
Author: Piotr Findeisen <piotr.findei...@gmail.com>
AuthorDate: Mon Aug 11 21:31:25 2025 +0200
Derive `WindowUDFImpl` equality, hash from `Eq`, `Hash` traits (#17081)
* Move `DynEq`, `DynHash` to `expr-common`
Use `DynEq` and `DynHash` traits from physical expressions crate to a
common crate for physical and logical expressions. This allows them to
be used by logical expressions.
* Derive WindowUDFImpl equality, hash from Eq, Hash traits
Previously, the `WindowUDFImpl` trait contained `equals` and
`hash_value` methods with contracts following the `Eq` and `Hash`
traits. However, the existence of default implementations of these
methods made it error-prone, with many functions (scalar, aggregate,
window) missing to customize the equals even though they ought to.
There is no fix to this that's not an API breaking change, so a breaking
change is warranted.
Removing the default implementations would be enough of a solution, but
at the cost of a lot of boilerplate needed in implementations.
Instead, this removes the methods from the trait, and reuses `DynEq`,
`DynHash` traits used previously only for physical expressions. This
allows for functions to provide their implementations using no more than
`#[derive(PartialEq, Eq, Hash)]` in a typical case.
* upgrade guide
* fix typo
* Fix PartialEq for WindowUDF impl
Wrong Any was compared
* Seal DynEq, DynHash
* Link to epic in upgrade guide
Co-authored-by: Andrew Lamb <and...@nerdnetworks.org>
---------
Co-authored-by: Andrew Lamb <and...@nerdnetworks.org>
---
datafusion-examples/examples/advanced_udwf.rs | 4 +-
.../user_defined/user_defined_window_functions.rs | 61 +++++++++----------
datafusion/expr-common/src/dyn_eq.rs | 64 ++++++++++++++++++++
datafusion/expr-common/src/lib.rs | 1 +
datafusion/expr/src/expr_fn.rs | 2 -
datafusion/expr/src/udf_eq.rs | 15 ++++-
datafusion/expr/src/udwf.rs | 70 +++++++---------------
datafusion/ffi/src/udwf/mod.rs | 3 -
datafusion/functions-window/src/cume_dist.rs | 2 +-
datafusion/functions-window/src/lead_lag.rs | 6 +-
datafusion/functions-window/src/macros.rs | 14 ++---
datafusion/functions-window/src/nth_value.rs | 6 +-
datafusion/functions-window/src/ntile.rs | 2 +-
datafusion/functions-window/src/rank.rs | 5 +-
datafusion/functions-window/src/row_number.rs | 2 +-
.../src/simplify_expressions/expr_simplifier.rs | 2 -
.../physical-expr-common/src/physical_expr.rs | 31 ++--------
datafusion/proto/tests/cases/mod.rs | 2 -
.../proto/tests/cases/roundtrip_logical_plan.rs | 2 +-
docs/source/library-user-guide/upgrading.md | 8 +++
20 files changed, 160 insertions(+), 142 deletions(-)
diff --git a/datafusion-examples/examples/advanced_udwf.rs
b/datafusion-examples/examples/advanced_udwf.rs
index e0fab7ee9f..5e7275af7d 100644
--- a/datafusion-examples/examples/advanced_udwf.rs
+++ b/datafusion-examples/examples/advanced_udwf.rs
@@ -43,7 +43,7 @@ use datafusion::prelude::*;
/// a function `partition_evaluator` that returns the `MyPartitionEvaluator`
instance.
///
/// To do so, we must implement the `WindowUDFImpl` trait.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct SmoothItUdf {
signature: Signature,
}
@@ -149,7 +149,7 @@ impl PartitionEvaluator for MyPartitionEvaluator {
}
/// This UDWF will show how to use the WindowUDFImpl::simplify() API
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct SimplifySmoothItUdf {
signature: Signature,
}
diff --git
a/datafusion/core/tests/user_defined/user_defined_window_functions.rs
b/datafusion/core/tests/user_defined/user_defined_window_functions.rs
index f0ff0cbb22..b3542f4da8 100644
--- a/datafusion/core/tests/user_defined/user_defined_window_functions.rs
+++ b/datafusion/core/tests/user_defined/user_defined_window_functions.rs
@@ -30,8 +30,7 @@ use datafusion::prelude::SessionContext;
use datafusion_common::exec_datafusion_err;
use datafusion_expr::ptr_eq::PtrEq;
use datafusion_expr::{
- udf_equals_hash, PartitionEvaluator, Signature, TypeSignature, Volatility,
WindowUDF,
- WindowUDFImpl,
+ PartitionEvaluator, Signature, TypeSignature, Volatility, WindowUDF,
WindowUDFImpl,
};
use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
use datafusion_functions_window_common::{
@@ -42,7 +41,7 @@ use datafusion_physical_expr::{
PhysicalExpr,
};
use std::collections::HashMap;
-use std::hash::{DefaultHasher, Hash, Hasher};
+use std::hash::{Hash, Hasher};
use std::{
any::Any,
ops::Range,
@@ -571,8 +570,6 @@ impl OddCounter {
fn field(&self, field_args: WindowUDFFieldArgs) ->
Result<FieldRef> {
Ok(Field::new(field_args.name(), DataType::Int64, true).into())
}
-
- udf_equals_hash!(WindowUDFImpl);
}
ctx.register_udwf(WindowUDF::from(SimpleWindowUDF::new(test_state)))
@@ -648,7 +645,7 @@ fn odd_count_arr(arr: &Int64Array, num_rows: usize) ->
ArrayRef {
Arc::new(array)
}
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq, Hash)]
struct VariadicWindowUDF {
signature: Signature,
}
@@ -770,6 +767,31 @@ struct MetadataBasedWindowUdf {
metadata: HashMap<String, String>,
}
+impl PartialEq for MetadataBasedWindowUdf {
+ fn eq(&self, other: &Self) -> bool {
+ let Self {
+ name,
+ signature,
+ metadata,
+ } = self;
+ name == &other.name
+ && signature == &other.signature
+ && metadata == &other.metadata
+ }
+}
+impl Eq for MetadataBasedWindowUdf {}
+impl Hash for MetadataBasedWindowUdf {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let Self {
+ name,
+ signature,
+ metadata: _, // unhashable
+ } = self;
+ name.hash(state);
+ signature.hash(state);
+ }
+}
+
impl MetadataBasedWindowUdf {
fn new(metadata: HashMap<String, String>) -> Self {
// The name we return must be unique. Otherwise we will not call
distinct
@@ -820,33 +842,6 @@ impl WindowUDFImpl for MetadataBasedWindowUdf {
.with_metadata(self.metadata.clone())
.into())
}
-
- fn equals(&self, other: &dyn WindowUDFImpl) -> bool {
- let Some(other) = other.as_any().downcast_ref::<Self>() else {
- return false;
- };
- let Self {
- name,
- signature,
- metadata,
- } = self;
- name == &other.name
- && signature == &other.signature
- && metadata == &other.metadata
- }
-
- fn hash_value(&self) -> u64 {
- let Self {
- name,
- signature,
- metadata: _, // unhashable
- } = self;
- let mut hasher = DefaultHasher::new();
- std::any::type_name::<Self>().hash(&mut hasher);
- name.hash(&mut hasher);
- signature.hash(&mut hasher);
- hasher.finish()
- }
}
#[derive(Debug)]
diff --git a/datafusion/expr-common/src/dyn_eq.rs
b/datafusion/expr-common/src/dyn_eq.rs
new file mode 100644
index 0000000000..e0ebcae487
--- /dev/null
+++ b/datafusion/expr-common/src/dyn_eq.rs
@@ -0,0 +1,64 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::hash::{Hash, Hasher};
+
+/// A dyn-compatible version of [`Eq`] trait.
+/// The implementation constraints for this trait are the same as for [`Eq`]:
+/// the implementation must be reflexive, symmetric, and transitive.
+/// Additionally, if two values can be compared with [`DynEq`] and
[`PartialEq`] then
+/// they must be [`DynEq`]-equal if and only if they are [`PartialEq`]-equal.
+/// It is therefore strongly discouraged to implement this trait for types
+/// that implement `PartialEq<Other>` or `Eq<Other>` for any type `Other`
other than `Self`.
+///
+/// Note: This trait should not be implemented directly. Implement `Eq` and
`Any` and use
+/// the blanket implementation.
+#[allow(private_bounds)]
+pub trait DynEq: private::EqSealed {
+ fn dyn_eq(&self, other: &dyn Any) -> bool;
+}
+
+impl<T: Eq + Any> private::EqSealed for T {}
+impl<T: Eq + Any> DynEq for T {
+ fn dyn_eq(&self, other: &dyn Any) -> bool {
+ other.downcast_ref::<Self>() == Some(self)
+ }
+}
+
+/// A dyn-compatible version of [`Hash`] trait.
+/// If two values are equal according to [`DynEq`], they must produce the same
hash value.
+///
+/// Note: This trait should not be implemented directly. Implement `Hash` and
`Any` and use
+/// the blanket implementation.
+#[allow(private_bounds)]
+pub trait DynHash: private::HashSealed {
+ fn dyn_hash(&self, _state: &mut dyn Hasher);
+}
+
+impl<T: Hash + Any> private::HashSealed for T {}
+impl<T: Hash + Any> DynHash for T {
+ fn dyn_hash(&self, mut state: &mut dyn Hasher) {
+ self.type_id().hash(&mut state);
+ self.hash(&mut state)
+ }
+}
+
+mod private {
+ pub(super) trait EqSealed {}
+ pub(super) trait HashSealed {}
+}
diff --git a/datafusion/expr-common/src/lib.rs
b/datafusion/expr-common/src/lib.rs
index 597bf7713b..f0bb6f9994 100644
--- a/datafusion/expr-common/src/lib.rs
+++ b/datafusion/expr-common/src/lib.rs
@@ -35,6 +35,7 @@
pub mod accumulator;
pub mod casts;
pub mod columnar_value;
+pub mod dyn_eq;
pub mod groups_accumulator;
pub mod interval_arithmetic;
pub mod operator;
diff --git a/datafusion/expr/src/expr_fn.rs b/datafusion/expr/src/expr_fn.rs
index 6e5cd068b3..fa2b17d77e 100644
--- a/datafusion/expr/src/expr_fn.rs
+++ b/datafusion/expr/src/expr_fn.rs
@@ -695,8 +695,6 @@ impl WindowUDFImpl for SimpleWindowUDF {
true,
)))
}
-
- udf_equals_hash!(WindowUDFImpl);
}
pub fn interval_year_month_lit(value: &str) -> Expr {
diff --git a/datafusion/expr/src/udf_eq.rs b/datafusion/expr/src/udf_eq.rs
index 75d6a41c36..12af38ee3f 100644
--- a/datafusion/expr/src/udf_eq.rs
+++ b/datafusion/expr/src/udf_eq.rs
@@ -17,7 +17,7 @@
use crate::{AggregateUDFImpl, ScalarUDFImpl, WindowUDFImpl};
use std::fmt::Debug;
-use std::hash::{Hash, Hasher};
+use std::hash::{DefaultHasher, Hash, Hasher};
use std::ops::Deref;
use std::sync::Arc;
@@ -97,7 +97,18 @@ macro_rules! impl_for_udf_eq {
impl_for_udf_eq!(dyn AggregateUDFImpl + '_);
impl_for_udf_eq!(dyn ScalarUDFImpl + '_);
-impl_for_udf_eq!(dyn WindowUDFImpl + '_);
+
+impl UdfPointer for Arc<dyn WindowUDFImpl + '_> {
+ fn equals(&self, other: &(dyn WindowUDFImpl + '_)) -> bool {
+ self.as_ref().dyn_eq(other.as_any())
+ }
+
+ fn hash_value(&self) -> u64 {
+ let hasher = &mut DefaultHasher::new();
+ self.as_ref().dyn_hash(hasher);
+ hasher.finish()
+ }
+}
#[cfg(test)]
mod tests {
diff --git a/datafusion/expr/src/udwf.rs b/datafusion/expr/src/udwf.rs
index 507e88aad0..fd28c90274 100644
--- a/datafusion/expr/src/udwf.rs
+++ b/datafusion/expr/src/udwf.rs
@@ -19,7 +19,7 @@
use arrow::compute::SortOptions;
use std::cmp::Ordering;
-use std::hash::{DefaultHasher, Hash, Hasher};
+use std::hash::{Hash, Hasher};
use std::{
any::Any,
fmt::{self, Debug, Display, Formatter},
@@ -31,11 +31,11 @@ use arrow::datatypes::{DataType, FieldRef};
use crate::expr::WindowFunction;
use crate::udf_eq::UdfEq;
use crate::{
- function::WindowFunctionSimplification, udf_equals_hash, Expr,
PartitionEvaluator,
- Signature,
+ function::WindowFunctionSimplification, Expr, PartitionEvaluator,
Signature,
};
use datafusion_common::{not_impl_err, Result};
use datafusion_doc::Documentation;
+use datafusion_expr_common::dyn_eq::{DynEq, DynHash};
use datafusion_functions_window_common::expr::ExpressionArgs;
use datafusion_functions_window_common::field::WindowUDFFieldArgs;
use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
@@ -82,7 +82,7 @@ impl Display for WindowUDF {
impl PartialEq for WindowUDF {
fn eq(&self, other: &Self) -> bool {
- self.inner.equals(other.inner.as_ref())
+ self.inner.dyn_eq(other.inner.as_any())
}
}
@@ -90,7 +90,7 @@ impl Eq for WindowUDF {}
impl Hash for WindowUDF {
fn hash<H: Hasher>(&self, state: &mut H) {
- self.inner.hash_value().hash(state)
+ self.inner.dyn_hash(state)
}
}
@@ -229,6 +229,10 @@ where
/// This trait exposes the full API for implementing user defined window
functions and
/// can be used to implement any function.
///
+/// While the trait depends on [`DynEq`] and [`DynHash`] traits, these should
not be
+/// implemented directly. Instead, implement [`Eq`] and [`Hash`] and leverage
the
+/// blanket implementations of [`DynEq`] and [`DynHash`].
+///
/// See [`advanced_udwf.rs`] for a full example with complete implementation
and
/// [`WindowUDF`] for other available options.
///
@@ -246,7 +250,7 @@ where
/// # use
datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
/// # use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
///
-/// #[derive(Debug, Clone)]
+/// #[derive(Debug, Clone, PartialEq, Eq, Hash)]
/// struct SmoothIt {
/// signature: Signature,
/// }
@@ -305,7 +309,7 @@ where
/// .build()
/// .unwrap();
/// ```
-pub trait WindowUDFImpl: Debug + Send + Sync {
+pub trait WindowUDFImpl: Debug + DynEq + DynHash + Send + Sync {
/// Returns this object as an [`Any`] trait object
fn as_any(&self) -> &dyn Any;
@@ -358,41 +362,6 @@ pub trait WindowUDFImpl: Debug + Send + Sync {
None
}
- /// Return true if this window UDF is equal to the other.
- ///
- /// Allows customizing the equality of window UDFs.
- /// *Must* be implemented explicitly if the UDF type has internal state.
- /// Must be consistent with [`Self::hash_value`] and follow the same rules
as [`Eq`]:
- ///
- /// - reflexive: `a.equals(a)`;
- /// - symmetric: `a.equals(b)` implies `b.equals(a)`;
- /// - transitive: `a.equals(b)` and `b.equals(c)` implies `a.equals(c)`.
- ///
- /// By default, compares type, [`Self::name`], [`Self::aliases`] and
[`Self::signature`].
- fn equals(&self, other: &dyn WindowUDFImpl) -> bool {
- self.as_any().type_id() == other.as_any().type_id()
- && self.name() == other.name()
- && self.aliases() == other.aliases()
- && self.signature() == other.signature()
- }
-
- /// Returns a hash value for this window UDF.
- ///
- /// Allows customizing the hash code of window UDFs.
- /// *Must* be implemented explicitly whenever [`Self::equals`] is
implemented.
- ///
- /// Similarly to [`Hash`] and [`Eq`], if [`Self::equals`] returns true for
two UDFs,
- /// their `hash_value`s must be the same.
- ///
- /// By default, it only hashes the type. The other fields are not hashed,
as usually the
- /// name, signature, and aliases are implied by the UDF type. Recall that
UDFs with state
- /// (and thus possibly changing fields) must override [`Self::equals`] and
[`Self::hash_value`].
- fn hash_value(&self) -> u64 {
- let hasher = &mut DefaultHasher::new();
- self.as_any().type_id().hash(hasher);
- hasher.finish()
- }
-
/// The [`FieldRef`] of the final result of evaluating this window
function.
///
/// Call `field_args.name()` to get the fully qualified name for defining
@@ -461,7 +430,7 @@ pub enum ReversedUDWF {
impl PartialEq for dyn WindowUDFImpl {
fn eq(&self, other: &Self) -> bool {
- self.equals(other)
+ self.dyn_eq(other.as_any())
}
}
@@ -533,8 +502,6 @@ impl WindowUDFImpl for AliasedWindowUDFImpl {
self.inner.simplify()
}
- udf_equals_hash!(WindowUDFImpl);
-
fn field(&self, field_args: WindowUDFFieldArgs) -> Result<FieldRef> {
self.inner.field(field_args)
}
@@ -598,7 +565,7 @@ mod test {
use std::any::Any;
use std::cmp::Ordering;
- #[derive(Debug, Clone)]
+ #[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct AWindowUDF {
signature: Signature,
}
@@ -637,7 +604,7 @@ mod test {
}
}
- #[derive(Debug, Clone)]
+ #[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct BWindowUDF {
signature: Signature,
}
@@ -676,6 +643,15 @@ mod test {
}
}
+ #[test]
+ fn test_partial_eq() {
+ let a1 = WindowUDF::from(AWindowUDF::new());
+ let a2 = WindowUDF::from(AWindowUDF::new());
+ let eq = a1 == a2;
+ assert!(eq);
+ assert_eq!(a1, a2);
+ }
+
#[test]
fn test_partial_ord() {
let a1 = WindowUDF::from(AWindowUDF::new());
diff --git a/datafusion/ffi/src/udwf/mod.rs b/datafusion/ffi/src/udwf/mod.rs
index a5e18cdf1e..d17999e274 100644
--- a/datafusion/ffi/src/udwf/mod.rs
+++ b/datafusion/ffi/src/udwf/mod.rs
@@ -25,7 +25,6 @@ use arrow::{
datatypes::{DataType, SchemaRef},
};
use arrow_schema::{Field, FieldRef};
-use datafusion::logical_expr::udf_equals_hash;
use datafusion::{
error::DataFusionError,
logical_expr::{
@@ -349,8 +348,6 @@ impl WindowUDFImpl for ForeignWindowUDF {
let options: Option<&FFI_SortOptions> =
self.udf.sort_options.as_ref().into();
options.map(|s| s.into())
}
-
- udf_equals_hash!(WindowUDFImpl);
}
#[repr(C)]
diff --git a/datafusion/functions-window/src/cume_dist.rs
b/datafusion/functions-window/src/cume_dist.rs
index 57ffa627eb..9b4c682f2e 100644
--- a/datafusion/functions-window/src/cume_dist.rs
+++ b/datafusion/functions-window/src/cume_dist.rs
@@ -63,7 +63,7 @@ FROM employees;
```
"#
)]
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq, Hash)]
pub struct CumeDist {
signature: Signature,
}
diff --git a/datafusion/functions-window/src/lead_lag.rs
b/datafusion/functions-window/src/lead_lag.rs
index 8f9a1a7a72..7950cc93f8 100644
--- a/datafusion/functions-window/src/lead_lag.rs
+++ b/datafusion/functions-window/src/lead_lag.rs
@@ -25,8 +25,8 @@ use datafusion_common::arrow::datatypes::Field;
use datafusion_common::{arrow_datafusion_err, DataFusionError, Result,
ScalarValue};
use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
use datafusion_expr::{
- udf_equals_hash, Documentation, Literal, PartitionEvaluator, ReversedUDWF,
Signature,
- TypeSignature, Volatility, WindowUDFImpl,
+ Documentation, Literal, PartitionEvaluator, ReversedUDWF, Signature,
TypeSignature,
+ Volatility, WindowUDFImpl,
};
use datafusion_functions_window_common::expr::ExpressionArgs;
use datafusion_functions_window_common::field::WindowUDFFieldArgs;
@@ -299,8 +299,6 @@ impl WindowUDFImpl for WindowShift {
WindowShiftKind::Lead => Some(get_lead_doc()),
}
}
-
- udf_equals_hash!(WindowUDFImpl);
}
/// When `lead`/`lag` is evaluated on a `NULL` expression we attempt to
diff --git a/datafusion/functions-window/src/macros.rs
b/datafusion/functions-window/src/macros.rs
index 23414a7a71..890ced90a9 100644
--- a/datafusion/functions-window/src/macros.rs
+++ b/datafusion/functions-window/src/macros.rs
@@ -57,7 +57,7 @@
/// #
/// # assert_eq!(simple_udwf().name(), "simple_user_defined_window_function");
/// #
-/// # #[derive(Debug)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct SimpleUDWF {
/// # signature: Signature,
/// # }
@@ -171,7 +171,7 @@ macro_rules! get_or_init_udwf {
/// # "row_number() ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED
FOLLOWING"
/// # );
/// #
-/// # #[derive(Debug)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct RowNumber {
/// # signature: Signature,
/// # }
@@ -250,7 +250,7 @@ macro_rules! get_or_init_udwf {
/// # "lead(a,Int64(1),NULL) ROWS BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING"
/// # );
/// #
-/// # #[derive(Debug)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct Lead {
/// # signature: Signature,
/// # }
@@ -379,7 +379,7 @@ macro_rules! create_udwf_expr {
/// #
/// # assert_eq!(simple_udwf().name(), "simple_user_defined_window_function");
/// #
-/// # #[derive(Debug)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct SimpleUDWF {
/// # signature: Signature,
/// # }
@@ -446,7 +446,7 @@ macro_rules! create_udwf_expr {
/// # "row_number() ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED
FOLLOWING"
/// # );
/// #
-/// # #[derive(Debug)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct RowNumber {
/// # signature: Signature,
/// # }
@@ -524,7 +524,7 @@ macro_rules! create_udwf_expr {
/// # "lead(a,Int64(1),NULL) ROWS BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING"
/// # );
/// #
-/// # #[derive(Debug)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct Lead {
/// # signature: Signature,
/// # }
@@ -614,7 +614,7 @@ macro_rules! create_udwf_expr {
/// # "lead(a,Int64(1),NULL) ROWS BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING"
/// # );
/// #
-/// # #[derive(Debug)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct Lead {
/// # signature: Signature,
/// # }
diff --git a/datafusion/functions-window/src/nth_value.rs
b/datafusion/functions-window/src/nth_value.rs
index 2da2fae23d..309978e9e7 100644
--- a/datafusion/functions-window/src/nth_value.rs
+++ b/datafusion/functions-window/src/nth_value.rs
@@ -26,8 +26,8 @@ use datafusion_common::{exec_datafusion_err, exec_err,
Result, ScalarValue};
use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
use datafusion_expr::window_state::WindowAggState;
use datafusion_expr::{
- udf_equals_hash, Documentation, Literal, PartitionEvaluator, ReversedUDWF,
Signature,
- TypeSignature, Volatility, WindowUDFImpl,
+ Documentation, Literal, PartitionEvaluator, ReversedUDWF, Signature,
TypeSignature,
+ Volatility, WindowUDFImpl,
};
use datafusion_functions_window_common::field;
use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
@@ -337,8 +337,6 @@ impl WindowUDFImpl for NthValue {
NthValueKind::Nth => Some(get_nth_value_doc()),
}
}
-
- udf_equals_hash!(WindowUDFImpl);
}
#[derive(Debug, Clone)]
diff --git a/datafusion/functions-window/src/ntile.rs
b/datafusion/functions-window/src/ntile.rs
index 91e36d8053..f8deac6b33 100644
--- a/datafusion/functions-window/src/ntile.rs
+++ b/datafusion/functions-window/src/ntile.rs
@@ -76,7 +76,7 @@ FROM employees;
```
"#
)]
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq, Hash)]
pub struct Ntile {
signature: Signature,
}
diff --git a/datafusion/functions-window/src/rank.rs
b/datafusion/functions-window/src/rank.rs
index e026bdf594..bc88572a92 100644
--- a/datafusion/functions-window/src/rank.rs
+++ b/datafusion/functions-window/src/rank.rs
@@ -29,8 +29,7 @@ use datafusion_common::utils::get_row_at_idx;
use datafusion_common::{exec_err, Result, ScalarValue};
use datafusion_expr::window_doc_sections::DOC_SECTION_RANKING;
use datafusion_expr::{
- udf_equals_hash, Documentation, PartitionEvaluator, Signature, Volatility,
- WindowUDFImpl,
+ Documentation, PartitionEvaluator, Signature, Volatility, WindowUDFImpl,
};
use datafusion_functions_window_common::field;
use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
@@ -241,8 +240,6 @@ impl WindowUDFImpl for Rank {
RankType::Percent => Some(get_percent_rank_doc()),
}
}
-
- udf_equals_hash!(WindowUDFImpl);
}
/// State for the RANK(rank) built-in window function.
diff --git a/datafusion/functions-window/src/row_number.rs
b/datafusion/functions-window/src/row_number.rs
index 19d076e052..2ef490c3c3 100644
--- a/datafusion/functions-window/src/row_number.rs
+++ b/datafusion/functions-window/src/row_number.rs
@@ -67,7 +67,7 @@ FROM employees;
```
"#
)]
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq, Hash)]
pub struct RowNumber {
signature: Signature,
}
diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
index 128e84018c..53d18e7edf 100644
--- a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
+++ b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
@@ -4385,8 +4385,6 @@ mod tests {
fn field(&self, _field_args: WindowUDFFieldArgs) -> Result<FieldRef> {
unimplemented!("not needed for tests")
}
-
- udf_equals_hash!(WindowUDFImpl);
}
#[derive(Debug)]
struct VolatileUdf {
diff --git a/datafusion/physical-expr-common/src/physical_expr.rs
b/datafusion/physical-expr-common/src/physical_expr.rs
index 90c7a1d514..ff39a851e2 100644
--- a/datafusion/physical-expr-common/src/physical_expr.rs
+++ b/datafusion/physical-expr-common/src/physical_expr.rs
@@ -381,40 +381,19 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug +
DynEq + DynHash {
}
}
-/// [`PhysicalExpr`] can't be constrained by [`Eq`] directly because it must
remain object
-/// safe. To ease implementation, blanket implementation is provided for
[`Eq`] types.
-pub trait DynEq {
- fn dyn_eq(&self, other: &dyn Any) -> bool;
-}
-
-impl<T: Eq + Any> DynEq for T {
- fn dyn_eq(&self, other: &dyn Any) -> bool {
- other.downcast_ref::<Self>() == Some(self)
- }
-}
+#[deprecated(
+ since = "50.0.0",
+ note = "Use `datafusion_expr_common::dyn_eq` instead"
+)]
+pub use datafusion_expr_common::dyn_eq::{DynEq, DynHash};
impl PartialEq for dyn PhysicalExpr {
fn eq(&self, other: &Self) -> bool {
self.dyn_eq(other.as_any())
}
}
-
impl Eq for dyn PhysicalExpr {}
-/// [`PhysicalExpr`] can't be constrained by [`Hash`] directly because it must
remain
-/// object safe. To ease implementation blanket implementation is provided for
[`Hash`]
-/// types.
-pub trait DynHash {
- fn dyn_hash(&self, _state: &mut dyn Hasher);
-}
-
-impl<T: Hash + Any> DynHash for T {
- fn dyn_hash(&self, mut state: &mut dyn Hasher) {
- self.type_id().hash(&mut state);
- self.hash(&mut state)
- }
-}
-
impl Hash for dyn PhysicalExpr {
fn hash<H: Hasher>(&self, state: &mut H) {
self.dyn_hash(state);
diff --git a/datafusion/proto/tests/cases/mod.rs
b/datafusion/proto/tests/cases/mod.rs
index ee5005fdde..105d94f1b4 100644
--- a/datafusion/proto/tests/cases/mod.rs
+++ b/datafusion/proto/tests/cases/mod.rs
@@ -177,8 +177,6 @@ impl WindowUDFImpl for CustomUDWF {
) -> datafusion_common::Result<FieldRef> {
Ok(Field::new(field_args.name(), DataType::UInt64, false).into())
}
-
- udf_equals_hash!(WindowUDFImpl);
}
#[derive(Debug)]
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index 96d4ea7642..5bd6448ed7 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -2495,7 +2495,7 @@ fn roundtrip_window() {
}
}
- #[derive(Debug, Clone)]
+ #[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct SimpleWindowUDF {
signature: Signature,
}
diff --git a/docs/source/library-user-guide/upgrading.md
b/docs/source/library-user-guide/upgrading.md
index 58167238fe..09e905f7ff 100644
--- a/docs/source/library-user-guide/upgrading.md
+++ b/docs/source/library-user-guide/upgrading.md
@@ -24,6 +24,14 @@
**Note:** DataFusion `50.0.0` has not been released yet. The information
provided in this section pertains to features and changes that have already
been merged to the main branch and are awaiting release in this version.
You can see the current [status of the `50.0.0 `release
here](https://github.com/apache/datafusion/issues/16799)
+### `WindowUDFImpl` trait now requires `PartialEq`, `Eq`, and `Hash` traits
+
+To address error-proneness of `WindowUDFImpl::equals` method and to make it
easy to implement function
+equality correctly, the `WindowUDFImpl::equals` and
`WindowUDFImpl::hash_value` methods have been replaced
+with the requirement to implement the `PartialEq`, `Eq`, and `Hash` traits on
any type implementing `WindowUDFImpl`. Please see [issue #16677] for more
details
+
+[issue #16677]: https://github.com/apache/datafusion/issues/16677
+
### `AsyncScalarUDFImpl::invoke_async_with_args` returns `ColumnarValue`
In order to enable single value optimizations and be consistent with other
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@datafusion.apache.org
For additional commands, e-mail: commits-h...@datafusion.apache.org
