diff --git a/datafusion/core/tests/sql/path_partition.rs b/datafusion/core/tests/sql/path_partition.rs index 919054e8330f..975984e5b11f 100644 --- a/datafusion/core/tests/sql/path_partition.rs +++ b/datafusion/core/tests/sql/path_partition.rs @@ -47,7 +47,6 @@ use bytes::Bytes; use chrono::{TimeZone, Utc}; use datafusion_expr::{col, lit, Expr, Operator}; use datafusion_physical_expr::expressions::{BinaryExpr, Column, Literal}; -use datafusion_physical_expr::PhysicalExpr; use futures::stream::{self, BoxStream}; use object_store::{ path::Path, GetOptions, GetResult, GetResultPayload, ListResult, ObjectMeta, @@ -97,7 +96,7 @@ async fn parquet_partition_pruning_filter() -> Result<()> { assert!(pred.as_any().is::()); let pred = pred.as_any().downcast_ref::().unwrap(); - assert_eq!(pred, expected.as_any()); + assert_eq!(pred, expected.as_ref()); Ok(()) } diff --git a/datafusion/physical-expr-common/src/physical_expr.rs b/datafusion/physical-expr-common/src/physical_expr.rs index cc725cf2cefb..aa816cfa4469 100644 --- a/datafusion/physical-expr-common/src/physical_expr.rs +++ b/datafusion/physical-expr-common/src/physical_expr.rs @@ -52,7 +52,7 @@ use datafusion_expr_common::sort_properties::ExprProperties; /// [`Expr`]: https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html /// [`create_physical_expr`]: https://docs.rs/datafusion/latest/datafusion/physical_expr/fn.create_physical_expr.html /// [`Column`]: https://docs.rs/datafusion/latest/datafusion/physical_expr/expressions/struct.Column.html -pub trait PhysicalExpr: Send + Sync + Display + Debug + PartialEq { +pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash { /// Returns the physical expression as [`Any`] so that it can be /// downcast to a specific implementation. fn as_any(&self) -> &dyn Any; @@ -141,38 +141,6 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + PartialEq { Ok(Some(vec![])) } - /// Update the hash `state` with this expression requirements from - /// [`Hash`]. - /// - /// This method is required to support hashing [`PhysicalExpr`]s. To - /// implement it, typically the type implementing - /// [`PhysicalExpr`] implements [`Hash`] and - /// then the following boiler plate is used: - /// - /// # Example: - /// ``` - /// // User defined expression that derives Hash - /// #[derive(Hash, Debug, PartialEq, Eq)] - /// struct MyExpr { - /// val: u64 - /// } - /// - /// // impl PhysicalExpr { - /// // ... - /// # impl MyExpr { - /// // Boiler plate to call the derived Hash impl - /// fn dyn_hash(&self, state: &mut dyn std::hash::Hasher) { - /// use std::hash::Hash; - /// let mut s = state; - /// self.hash(&mut s); - /// } - /// // } - /// # } - /// ``` - /// Note: [`PhysicalExpr`] is not constrained by [`Hash`] - /// directly because it must remain object safe. - fn dyn_hash(&self, _state: &mut dyn Hasher); - /// Calculates the properties of this [`PhysicalExpr`] based on its /// children's properties (i.e. order and range), recursively aggregating /// the information from its children. In cases where the [`PhysicalExpr`] @@ -183,6 +151,42 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + PartialEq { } } +/// [`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 DynEq for T { + fn dyn_eq(&self, other: &dyn Any) -> bool { + other + .downcast_ref::() + .map_or(false, |other| other == self) + } +} + +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 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(&self, state: &mut H) { self.dyn_hash(state); @@ -210,20 +214,6 @@ pub fn with_new_children_if_necessary( } } -pub fn down_cast_any_ref(any: &dyn Any) -> &dyn Any { - if any.is::>() { - any.downcast_ref::>() - .unwrap() - .as_any() - } else if any.is::>() { - any.downcast_ref::>() - .unwrap() - .as_any() - } else { - any - } -} - /// Returns [`Display`] able a list of [`PhysicalExpr`] /// /// Example output: `[a + 1, b]` diff --git a/datafusion/physical-expr-common/src/sort_expr.rs b/datafusion/physical-expr-common/src/sort_expr.rs index d825bfe7e264..c08dcea0d1a7 100644 --- a/datafusion/physical-expr-common/src/sort_expr.rs +++ b/datafusion/physical-expr-common/src/sort_expr.rs @@ -56,14 +56,10 @@ use datafusion_expr_common::columnar_value::ColumnarValue; /// # fn evaluate(&self, batch: &RecordBatch) -> Result {todo!() } /// # fn children(&self) -> Vec<&Arc> {todo!()} /// # fn with_new_children(self: Arc, children: Vec>) -> Result> {todo!()} -/// # fn dyn_hash(&self, _state: &mut dyn Hasher) {todo!()} /// # } /// # impl Display for MyPhysicalExpr { /// # fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { write!(f, "a") } /// # } -/// # impl PartialEq for MyPhysicalExpr { -/// # fn eq(&self, _other: &dyn Any) -> bool { true } -/// # } /// # fn col(name: &str) -> Arc { Arc::new(MyPhysicalExpr) } /// // Sort by a ASC /// let options = SortOptions::default(); diff --git a/datafusion/physical-expr/src/equivalence/class.rs b/datafusion/physical-expr/src/equivalence/class.rs index c1851ddb22b5..a2dc25473b6f 100644 --- a/datafusion/physical-expr/src/equivalence/class.rs +++ b/datafusion/physical-expr/src/equivalence/class.rs @@ -67,8 +67,7 @@ pub struct ConstExpr { impl PartialEq for ConstExpr { fn eq(&self, other: &Self) -> bool { - self.across_partitions == other.across_partitions - && self.expr.eq(other.expr.as_any()) + self.across_partitions == other.across_partitions && self.expr.eq(&other.expr) } } @@ -121,7 +120,7 @@ impl ConstExpr { /// Returns true if this constant expression is equal to the given expression pub fn eq_expr(&self, other: impl AsRef) -> bool { - self.expr.eq(other.as_ref().as_any()) + self.expr.as_ref() == other.as_ref() } /// Returns a [`Display`]able list of `ConstExpr`. @@ -557,7 +556,7 @@ impl EquivalenceGroup { new_classes.push((source, vec![Arc::clone(target)])); } if let Some((_, values)) = - new_classes.iter_mut().find(|(key, _)| key.eq(source)) + new_classes.iter_mut().find(|(key, _)| *key == source) { if !physical_exprs_contains(values, target) { values.push(Arc::clone(target)); diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index 47b04a876b37..ae2bfe5b0bd4 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -17,11 +17,10 @@ mod kernels; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::{any::Any, sync::Arc}; use crate::intervals::cp_solver::{propagate_arithmetic, propagate_comparison}; -use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use arrow::array::*; @@ -48,7 +47,7 @@ use kernels::{ }; /// Binary expression -#[derive(Debug, Hash, Clone)] +#[derive(Debug, Clone, Eq)] pub struct BinaryExpr { left: Arc, op: Operator, @@ -57,6 +56,24 @@ pub struct BinaryExpr { fail_on_overflow: bool, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for BinaryExpr { + fn eq(&self, other: &Self) -> bool { + self.left.eq(&other.left) + && self.op.eq(&other.op) + && self.right.eq(&other.right) + && self.fail_on_overflow.eq(&other.fail_on_overflow) + } +} +impl Hash for BinaryExpr { + fn hash(&self, state: &mut H) { + self.left.hash(state); + self.op.hash(state); + self.right.hash(state); + self.fail_on_overflow.hash(state); + } +} + impl BinaryExpr { /// Create new binary expression pub fn new( @@ -477,11 +494,6 @@ impl PhysicalExpr for BinaryExpr { } } - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } - /// For each operator, [`BinaryExpr`] has distinct rules. /// TODO: There may be rules specific to some data types and expression ranges. fn get_properties(&self, children: &[ExprProperties]) -> Result { @@ -525,20 +537,6 @@ impl PhysicalExpr for BinaryExpr { } } -impl PartialEq for BinaryExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| { - self.left.eq(&x.left) - && self.op == x.op - && self.right.eq(&x.right) - && self.fail_on_overflow.eq(&x.fail_on_overflow) - }) - .unwrap_or(false) - } -} - /// Casts dictionary array to result type for binary numerical operators. Such operators /// between array and scalar produce a dictionary array other than primitive array of the /// same operators between array and array. This leads to inconsistent result types causing diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index 981e49d73750..0e307153341b 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -16,11 +16,10 @@ // under the License. use std::borrow::Cow; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::{any::Any, sync::Arc}; use crate::expressions::try_cast; -use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use arrow::array::*; @@ -37,7 +36,7 @@ use itertools::Itertools; type WhenThen = (Arc, Arc); -#[derive(Debug, Hash)] +#[derive(Debug, Hash, PartialEq, Eq)] enum EvalMethod { /// CASE WHEN condition THEN result /// [WHEN ...] @@ -80,7 +79,7 @@ enum EvalMethod { /// [WHEN ...] /// [ELSE result] /// END -#[derive(Debug, Hash)] +#[derive(Debug, Hash, PartialEq, Eq)] pub struct CaseExpr { /// Optional base expression that can be compared to literal values in the "when" expressions expr: Option>, @@ -506,39 +505,6 @@ impl PhysicalExpr for CaseExpr { )?)) } } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } -} - -impl PartialEq for CaseExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| { - let expr_eq = match (&self.expr, &x.expr) { - (Some(expr1), Some(expr2)) => expr1.eq(expr2), - (None, None) => true, - _ => false, - }; - let else_expr_eq = match (&self.else_expr, &x.else_expr) { - (Some(expr1), Some(expr2)) => expr1.eq(expr2), - (None, None) => true, - _ => false, - }; - expr_eq - && else_expr_eq - && self.when_then_expr.len() == x.when_then_expr.len() - && self.when_then_expr.iter().zip(x.when_then_expr.iter()).all( - |((when1, then1), (when2, then2))| { - when1.eq(when2) && then1.eq(then2) - }, - ) - }) - .unwrap_or(false) - } } /// Create a CASE expression diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index 457c47097a19..7eda5fb4beaa 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -17,10 +17,10 @@ use std::any::Any; use std::fmt; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; -use crate::physical_expr::{down_cast_any_ref, PhysicalExpr}; +use crate::physical_expr::PhysicalExpr; use arrow::compute::{can_cast_types, CastOptions}; use arrow::datatypes::{DataType, DataType::*, Schema}; @@ -42,7 +42,7 @@ const DEFAULT_SAFE_CAST_OPTIONS: CastOptions<'static> = CastOptions { }; /// CAST expression casts an expression to a specific data type and returns a runtime error on invalid cast -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Eq)] pub struct CastExpr { /// The expression to cast pub expr: Arc, @@ -52,6 +52,23 @@ pub struct CastExpr { cast_options: CastOptions<'static>, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for CastExpr { + fn eq(&self, other: &Self) -> bool { + self.expr.eq(&other.expr) + && self.cast_type.eq(&other.cast_type) + && self.cast_options.eq(&other.cast_options) + } +} + +impl Hash for CastExpr { + fn hash(&self, state: &mut H) { + self.expr.hash(state); + self.cast_type.hash(state); + self.cast_options.hash(state); + } +} + impl CastExpr { /// Create a new CastExpr pub fn new( @@ -160,13 +177,6 @@ impl PhysicalExpr for CastExpr { ])) } - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.expr.hash(&mut s); - self.cast_type.hash(&mut s); - self.cast_options.hash(&mut s); - } - /// A [`CastExpr`] preserves the ordering of its child if the cast is done /// under the same datatype family. fn get_properties(&self, children: &[ExprProperties]) -> Result { @@ -186,19 +196,6 @@ impl PhysicalExpr for CastExpr { } } -impl PartialEq for CastExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| { - self.expr.eq(&x.expr) - && self.cast_type == x.cast_type - && self.cast_options == x.cast_options - }) - .unwrap_or(false) - } -} - /// Return a PhysicalExpression representing `expr` casted to /// `cast_type`, if any casting is needed. /// diff --git a/datafusion/physical-expr/src/expressions/column.rs b/datafusion/physical-expr/src/expressions/column.rs index 3e2d49e9fa69..5f6932f6d725 100644 --- a/datafusion/physical-expr/src/expressions/column.rs +++ b/datafusion/physical-expr/src/expressions/column.rs @@ -18,9 +18,10 @@ //! Physical column reference: [`Column`] use std::any::Any; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; +use crate::physical_expr::PhysicalExpr; use arrow::{ datatypes::{DataType, Schema}, record_batch::RecordBatch, @@ -30,8 +31,6 @@ use datafusion_common::tree_node::{Transformed, TreeNode}; use datafusion_common::{internal_err, plan_err, Result}; use datafusion_expr::ColumnarValue; -use crate::physical_expr::{down_cast_any_ref, PhysicalExpr}; - /// Represents the column at a given index in a RecordBatch /// /// This is a physical expression that represents a column at a given index in an @@ -139,20 +138,6 @@ impl PhysicalExpr for Column { ) -> Result> { Ok(self) } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } -} - -impl PartialEq for Column { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self == x) - .unwrap_or(false) - } } impl Column { diff --git a/datafusion/physical-expr/src/expressions/in_list.rs b/datafusion/physical-expr/src/expressions/in_list.rs index cf57ce3e0e21..e42a4bb3a14d 100644 --- a/datafusion/physical-expr/src/expressions/in_list.rs +++ b/datafusion/physical-expr/src/expressions/in_list.rs @@ -22,7 +22,7 @@ use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::sync::Arc; -use crate::physical_expr::{down_cast_any_ref, physical_exprs_bag_equal}; +use crate::physical_expr::physical_exprs_bag_equal; use crate::PhysicalExpr; use arrow::array::*; @@ -398,26 +398,24 @@ impl PhysicalExpr for InListExpr { self.static_filter.clone(), ))) } +} - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.expr.hash(&mut s); - self.negated.hash(&mut s); - self.list.hash(&mut s); - // Add `self.static_filter` when hash is available +impl PartialEq for InListExpr { + fn eq(&self, other: &Self) -> bool { + self.expr.eq(&other.expr) + && physical_exprs_bag_equal(&self.list, &other.list) + && self.negated == other.negated } } -impl PartialEq for InListExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| { - self.expr.eq(&x.expr) - && physical_exprs_bag_equal(&self.list, &x.list) - && self.negated == x.negated - }) - .unwrap_or(false) +impl Eq for InListExpr {} + +impl Hash for InListExpr { + fn hash(&self, state: &mut H) { + self.expr.hash(state); + self.negated.hash(state); + self.list.hash(state); + // Add `self.static_filter` when hash is available } } diff --git a/datafusion/physical-expr/src/expressions/is_not_null.rs b/datafusion/physical-expr/src/expressions/is_not_null.rs index cbab7d0c9d1f..4930865f4c98 100644 --- a/datafusion/physical-expr/src/expressions/is_not_null.rs +++ b/datafusion/physical-expr/src/expressions/is_not_null.rs @@ -17,10 +17,9 @@ //! IS NOT NULL expression -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::{any::Any, sync::Arc}; -use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use arrow::{ datatypes::{DataType, Schema}, @@ -31,12 +30,25 @@ use datafusion_common::ScalarValue; use datafusion_expr::ColumnarValue; /// IS NOT NULL expression -#[derive(Debug, Hash)] +#[derive(Debug, Eq)] pub struct IsNotNullExpr { /// The input expression arg: Arc, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for IsNotNullExpr { + fn eq(&self, other: &Self) -> bool { + self.arg.eq(&other.arg) + } +} + +impl Hash for IsNotNullExpr { + fn hash(&self, state: &mut H) { + self.arg.hash(state); + } +} + impl IsNotNullExpr { /// Create new not expression pub fn new(arg: Arc) -> Self { @@ -92,21 +104,8 @@ impl PhysicalExpr for IsNotNullExpr { ) -> Result> { Ok(Arc::new(IsNotNullExpr::new(Arc::clone(&children[0])))) } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } } -impl PartialEq for IsNotNullExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self.arg.eq(&x.arg)) - .unwrap_or(false) - } -} /// Create an IS NOT NULL expression pub fn is_not_null(arg: Arc) -> Result> { Ok(Arc::new(IsNotNullExpr::new(arg))) diff --git a/datafusion/physical-expr/src/expressions/is_null.rs b/datafusion/physical-expr/src/expressions/is_null.rs index 1c8597d3fdea..6a02d5ecc1f2 100644 --- a/datafusion/physical-expr/src/expressions/is_null.rs +++ b/datafusion/physical-expr/src/expressions/is_null.rs @@ -17,27 +17,38 @@ //! IS NULL expression -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::{any::Any, sync::Arc}; +use crate::PhysicalExpr; use arrow::{ datatypes::{DataType, Schema}, record_batch::RecordBatch, }; - -use crate::physical_expr::down_cast_any_ref; -use crate::PhysicalExpr; use datafusion_common::Result; use datafusion_common::ScalarValue; use datafusion_expr::ColumnarValue; /// IS NULL expression -#[derive(Debug, Hash)] +#[derive(Debug, Eq)] pub struct IsNullExpr { /// Input expression arg: Arc, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for IsNullExpr { + fn eq(&self, other: &Self) -> bool { + self.arg.eq(&other.arg) + } +} + +impl Hash for IsNullExpr { + fn hash(&self, state: &mut H) { + self.arg.hash(state); + } +} + impl IsNullExpr { /// Create new not expression pub fn new(arg: Arc) -> Self { @@ -92,20 +103,6 @@ impl PhysicalExpr for IsNullExpr { ) -> Result> { Ok(Arc::new(IsNullExpr::new(Arc::clone(&children[0])))) } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } -} - -impl PartialEq for IsNullExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self.arg.eq(&x.arg)) - .unwrap_or(false) - } } /// Create an IS NULL expression diff --git a/datafusion/physical-expr/src/expressions/like.rs b/datafusion/physical-expr/src/expressions/like.rs index b84ba82b642d..d61cd63c35b1 100644 --- a/datafusion/physical-expr/src/expressions/like.rs +++ b/datafusion/physical-expr/src/expressions/like.rs @@ -15,11 +15,10 @@ // specific language governing permissions and limitations // under the License. -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::{any::Any, sync::Arc}; -use crate::{physical_expr::down_cast_any_ref, PhysicalExpr}; - +use crate::PhysicalExpr; use arrow::record_batch::RecordBatch; use arrow_schema::{DataType, Schema}; use datafusion_common::{internal_err, Result}; @@ -27,7 +26,7 @@ use datafusion_expr::ColumnarValue; use datafusion_physical_expr_common::datum::apply_cmp; // Like expression -#[derive(Debug, Hash)] +#[derive(Debug, Eq)] pub struct LikeExpr { negated: bool, case_insensitive: bool, @@ -35,6 +34,25 @@ pub struct LikeExpr { pattern: Arc, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for LikeExpr { + fn eq(&self, other: &Self) -> bool { + self.negated == other.negated + && self.case_insensitive == other.case_insensitive + && self.expr.eq(&other.expr) + && self.pattern.eq(&other.pattern) + } +} + +impl Hash for LikeExpr { + fn hash(&self, state: &mut H) { + self.negated.hash(state); + self.case_insensitive.hash(state); + self.expr.hash(state); + self.pattern.hash(state); + } +} + impl LikeExpr { pub fn new( negated: bool, @@ -127,25 +145,6 @@ impl PhysicalExpr for LikeExpr { Arc::clone(&children[1]), ))) } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } -} - -impl PartialEq for LikeExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| { - self.negated == x.negated - && self.case_insensitive == x.case_insensitive - && self.expr.eq(&x.expr) - && self.pattern.eq(&x.pattern) - }) - .unwrap_or(false) - } } /// used for optimize Dictionary like diff --git a/datafusion/physical-expr/src/expressions/literal.rs b/datafusion/physical-expr/src/expressions/literal.rs index ed24e9028153..f0d02eb605b2 100644 --- a/datafusion/physical-expr/src/expressions/literal.rs +++ b/datafusion/physical-expr/src/expressions/literal.rs @@ -18,10 +18,10 @@ //! Literal expressions for physical operations use std::any::Any; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; -use crate::physical_expr::{down_cast_any_ref, PhysicalExpr}; +use crate::physical_expr::PhysicalExpr; use arrow::{ datatypes::{DataType, Schema}, @@ -86,11 +86,6 @@ impl PhysicalExpr for Literal { Ok(self) } - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } - fn get_properties(&self, _children: &[ExprProperties]) -> Result { Ok(ExprProperties { sort_properties: SortProperties::Singleton, @@ -99,15 +94,6 @@ impl PhysicalExpr for Literal { } } -impl PartialEq for Literal { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self == x) - .unwrap_or(false) - } -} - /// Create a literal expression pub fn lit(value: T) -> Arc { match value.lit() { diff --git a/datafusion/physical-expr/src/expressions/negative.rs b/datafusion/physical-expr/src/expressions/negative.rs index 399ebde9f726..6235845fc028 100644 --- a/datafusion/physical-expr/src/expressions/negative.rs +++ b/datafusion/physical-expr/src/expressions/negative.rs @@ -18,10 +18,9 @@ //! Negation (-) expression use std::any::Any; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; -use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use arrow::{ @@ -38,12 +37,25 @@ use datafusion_expr::{ }; /// Negative expression -#[derive(Debug, Hash)] +#[derive(Debug, Eq)] pub struct NegativeExpr { /// Input expression arg: Arc, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for NegativeExpr { + fn eq(&self, other: &Self) -> bool { + self.arg.eq(&other.arg) + } +} + +impl Hash for NegativeExpr { + fn hash(&self, state: &mut H) { + self.arg.hash(state); + } +} + impl NegativeExpr { /// Create new not expression pub fn new(arg: Arc) -> Self { @@ -100,11 +112,6 @@ impl PhysicalExpr for NegativeExpr { Ok(Arc::new(NegativeExpr::new(Arc::clone(&children[0])))) } - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } - /// Given the child interval of a NegativeExpr, it calculates the NegativeExpr's interval. /// It replaces the upper and lower bounds after multiplying them with -1. /// Ex: `(a, b]` => `[-b, -a)` @@ -142,15 +149,6 @@ impl PhysicalExpr for NegativeExpr { } } -impl PartialEq for NegativeExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self.arg.eq(&x.arg)) - .unwrap_or(false) - } -} - /// Creates a unary expression NEGATIVE /// /// # Errors @@ -224,9 +222,7 @@ mod tests { #[test] fn test_evaluate_bounds() -> Result<()> { - let negative_expr = NegativeExpr { - arg: Arc::new(Column::new("a", 0)), - }; + let negative_expr = NegativeExpr::new(Arc::new(Column::new("a", 0))); let child_interval = Interval::make(Some(-2), Some(1))?; let negative_expr_interval = Interval::make(Some(-1), Some(2))?; assert_eq!( @@ -238,9 +234,7 @@ mod tests { #[test] fn test_propagate_constraints() -> Result<()> { - let negative_expr = NegativeExpr { - arg: Arc::new(Column::new("a", 0)), - }; + let negative_expr = NegativeExpr::new(Arc::new(Column::new("a", 0))); let original_child_interval = Interval::make(Some(-2), Some(3))?; let negative_expr_interval = Interval::make(Some(0), Some(4))?; let after_propagation = Some(vec![Interval::make(Some(-2), Some(0))?]); diff --git a/datafusion/physical-expr/src/expressions/no_op.rs b/datafusion/physical-expr/src/expressions/no_op.rs index 9148cb7c1c1d..c17b52f5cdff 100644 --- a/datafusion/physical-expr/src/expressions/no_op.rs +++ b/datafusion/physical-expr/src/expressions/no_op.rs @@ -18,7 +18,7 @@ //! NoOp placeholder for physical operations use std::any::Any; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; use arrow::{ @@ -26,7 +26,6 @@ use arrow::{ record_batch::RecordBatch, }; -use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use datafusion_common::{internal_err, Result}; use datafusion_expr::ColumnarValue; @@ -78,18 +77,4 @@ impl PhysicalExpr for NoOp { ) -> Result> { Ok(self) } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } -} - -impl PartialEq for NoOp { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self == x) - .unwrap_or(false) - } } diff --git a/datafusion/physical-expr/src/expressions/not.rs b/datafusion/physical-expr/src/expressions/not.rs index 6d91e9dfdd36..cc35c91c98bc 100644 --- a/datafusion/physical-expr/src/expressions/not.rs +++ b/datafusion/physical-expr/src/expressions/not.rs @@ -19,10 +19,9 @@ use std::any::Any; use std::fmt; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; -use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use arrow::datatypes::{DataType, Schema}; use arrow::record_batch::RecordBatch; @@ -31,12 +30,25 @@ use datafusion_expr::interval_arithmetic::Interval; use datafusion_expr::ColumnarValue; /// Not expression -#[derive(Debug, Hash)] +#[derive(Debug, Eq)] pub struct NotExpr { /// Input expression arg: Arc, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for NotExpr { + fn eq(&self, other: &Self) -> bool { + self.arg.eq(&other.arg) + } +} + +impl Hash for NotExpr { + fn hash(&self, state: &mut H) { + self.arg.hash(state); + } +} + impl NotExpr { /// Create new not expression pub fn new(arg: Arc) -> Self { @@ -100,24 +112,9 @@ impl PhysicalExpr for NotExpr { ) -> Result> { Ok(Arc::new(NotExpr::new(Arc::clone(&children[0])))) } - fn evaluate_bounds(&self, children: &[&Interval]) -> Result { children[0].not() } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } -} - -impl PartialEq for NotExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self.arg.eq(&x.arg)) - .unwrap_or(false) - } } /// Creates a unary expression NOT diff --git a/datafusion/physical-expr/src/expressions/try_cast.rs b/datafusion/physical-expr/src/expressions/try_cast.rs index 43b6c993d2b2..06f4e929992e 100644 --- a/datafusion/physical-expr/src/expressions/try_cast.rs +++ b/datafusion/physical-expr/src/expressions/try_cast.rs @@ -17,10 +17,9 @@ use std::any::Any; use std::fmt; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; -use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use arrow::compute; use arrow::compute::{cast_with_options, CastOptions}; @@ -32,7 +31,7 @@ use datafusion_common::{not_impl_err, Result, ScalarValue}; use datafusion_expr::ColumnarValue; /// TRY_CAST expression casts an expression to a specific data type and returns NULL on invalid cast -#[derive(Debug, Hash)] +#[derive(Debug, Eq)] pub struct TryCastExpr { /// The expression to cast expr: Arc, @@ -40,6 +39,20 @@ pub struct TryCastExpr { cast_type: DataType, } +// Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 +impl PartialEq for TryCastExpr { + fn eq(&self, other: &Self) -> bool { + self.expr.eq(&other.expr) && self.cast_type == other.cast_type + } +} + +impl Hash for TryCastExpr { + fn hash(&self, state: &mut H) { + self.expr.hash(state); + self.cast_type.hash(state); + } +} + impl TryCastExpr { /// Create a new CastExpr pub fn new(expr: Arc, cast_type: DataType) -> Self { @@ -110,20 +123,6 @@ impl PhysicalExpr for TryCastExpr { self.cast_type.clone(), ))) } - - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); - } -} - -impl PartialEq for TryCastExpr { - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| self.expr.eq(&x.expr) && self.cast_type == x.cast_type) - .unwrap_or(false) - } } /// Return a PhysicalExpression representing `expr` casted to diff --git a/datafusion/physical-expr/src/expressions/unknown_column.rs b/datafusion/physical-expr/src/expressions/unknown_column.rs index 590efd577963..a63caf7e1305 100644 --- a/datafusion/physical-expr/src/expressions/unknown_column.rs +++ b/datafusion/physical-expr/src/expressions/unknown_column.rs @@ -30,7 +30,7 @@ use arrow::{ use datafusion_common::{internal_err, Result}; use datafusion_expr::ColumnarValue; -#[derive(Debug, Hash, PartialEq, Eq, Clone)] +#[derive(Debug, Clone, Eq)] pub struct UnKnownColumn { name: String, } @@ -86,15 +86,16 @@ impl PhysicalExpr for UnKnownColumn { ) -> Result> { Ok(self) } +} - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.hash(&mut s); +impl Hash for UnKnownColumn { + fn hash(&self, state: &mut H) { + self.name.hash(state); } } -impl PartialEq for UnKnownColumn { - fn eq(&self, _other: &dyn Any) -> bool { +impl PartialEq for UnKnownColumn { + fn eq(&self, _other: &Self) -> bool { // UnknownColumn is not a valid expression, so it should not be equal to any other expression. // See https://github.com/apache/datafusion/pull/11536 false diff --git a/datafusion/physical-expr/src/physical_expr.rs b/datafusion/physical-expr/src/physical_expr.rs index c718e6b054ef..610c3656998b 100644 --- a/datafusion/physical-expr/src/physical_expr.rs +++ b/datafusion/physical-expr/src/physical_expr.rs @@ -20,8 +20,6 @@ use std::sync::Arc; pub(crate) use datafusion_physical_expr_common::physical_expr::PhysicalExpr; use itertools::izip; -pub use datafusion_physical_expr_common::physical_expr::down_cast_any_ref; - /// Shared [`PhysicalExpr`]. pub type PhysicalExprRef = Arc; diff --git a/datafusion/physical-expr/src/scalar_function.rs b/datafusion/physical-expr/src/scalar_function.rs index ab53106f6059..9bf168e8a199 100644 --- a/datafusion/physical-expr/src/scalar_function.rs +++ b/datafusion/physical-expr/src/scalar_function.rs @@ -31,10 +31,9 @@ use std::any::Any; use std::fmt::{self, Debug, Formatter}; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::sync::Arc; -use crate::physical_expr::{down_cast_any_ref, physical_exprs_equal}; use crate::PhysicalExpr; use arrow::datatypes::{DataType, Schema}; @@ -47,6 +46,7 @@ use datafusion_expr::type_coercion::functions::data_types_with_scalar_udf; use datafusion_expr::{expr_vec_fmt, ColumnarValue, Expr, ScalarUDF}; /// Physical expression of a scalar function +#[derive(Eq, PartialEq, Hash)] pub struct ScalarFunctionExpr { fun: Arc, name: String, @@ -194,14 +194,6 @@ impl PhysicalExpr for ScalarFunctionExpr { self.fun.propagate_constraints(interval, children) } - fn dyn_hash(&self, state: &mut dyn Hasher) { - let mut s = state; - self.name.hash(&mut s); - self.args.hash(&mut s); - self.return_type.hash(&mut s); - // Add `self.fun` when hash is available - } - fn get_properties(&self, children: &[ExprProperties]) -> Result { let sort_properties = self.fun.output_ordering(children)?; let children_range = children @@ -217,20 +209,6 @@ impl PhysicalExpr for ScalarFunctionExpr { } } -impl PartialEq for ScalarFunctionExpr { - /// Comparing name, args and return_type - fn eq(&self, other: &dyn Any) -> bool { - down_cast_any_ref(other) - .downcast_ref::() - .map(|x| { - self.name == x.name - && physical_exprs_equal(&self.args, &x.args) - && self.return_type == x.return_type - }) - .unwrap_or(false) - } -} - /// Create a physical expression for the UDF. pub fn create_physical_expr( fun: &ScalarUDF, diff --git a/datafusion/physical-expr/src/utils/mod.rs b/datafusion/physical-expr/src/utils/mod.rs index 4bd022975ac3..52384f5c411c 100644 --- a/datafusion/physical-expr/src/utils/mod.rs +++ b/datafusion/physical-expr/src/utils/mod.rs @@ -529,7 +529,7 @@ pub(crate) mod tests { ) .unwrap(); - assert_eq!(actual.as_ref(), expected.as_any()); + assert_eq!(actual.as_ref(), expected.as_ref()); } #[test] diff --git a/datafusion/proto/tests/cases/roundtrip_physical_plan.rs b/datafusion/proto/tests/cases/roundtrip_physical_plan.rs index 4a9bf6afb49e..90f0ef4db43b 100644 --- a/datafusion/proto/tests/cases/roundtrip_physical_plan.rs +++ b/datafusion/proto/tests/cases/roundtrip_physical_plan.rs @@ -17,7 +17,6 @@ use std::any::Any; use std::fmt::Display; -use std::hash::Hasher; use std::ops::Deref; use std::sync::Arc; use std::vec; @@ -747,23 +746,30 @@ fn roundtrip_parquet_exec_with_custom_predicate_expr() -> Result<()> { output_ordering: vec![], }; - #[derive(Debug, Hash, Clone)] + #[derive(Debug, Clone, Eq)] struct CustomPredicateExpr { inner: Arc, } + + // Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 + impl PartialEq for CustomPredicateExpr { + fn eq(&self, other: &Self) -> bool { + self.inner.eq(&other.inner) + } + } + + impl std::hash::Hash for CustomPredicateExpr { + fn hash(&self, state: &mut H) { + self.inner.hash(state); + } + } + impl Display for CustomPredicateExpr { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "CustomPredicateExpr") } } - impl PartialEq for CustomPredicateExpr { - fn eq(&self, other: &dyn Any) -> bool { - other - .downcast_ref::() - .map(|x| self.inner.eq(&x.inner)) - .unwrap_or(false) - } - } + impl PhysicalExpr for CustomPredicateExpr { fn as_any(&self) -> &dyn Any { self @@ -791,10 +797,6 @@ fn roundtrip_parquet_exec_with_custom_predicate_expr() -> Result<()> { ) -> Result> { todo!() } - - fn dyn_hash(&self, _state: &mut dyn Hasher) { - unreachable!() - } } #[derive(Debug)]