You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by fi...@apache.org on 2025/08/08 19:08:05 UTC
(datafusion) branch main updated: Fix hash/equality issues for ScalarFunctionExpr (#17078)
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 5f95734e1e Fix hash/equality issues for ScalarFunctionExpr (#17078)
5f95734e1e is described below
commit 5f95734e1e71493991b4150e327c63cdb974db1e
Author: Piotr Findeisen <pi...@gmail.com>
AuthorDate: Fri Aug 8 21:08:01 2025 +0200
Fix hash/equality issues for ScalarFunctionExpr (#17078)
* Fix equality impl for ScalarFunctionExpr
The equality implementation for ScalarFunctionExpr could incorrectly
return true when one compared object has more config options. This is
because the `iterator.zip(other)` terminates whenever one of the zipped
iterators terminates.
* Fix hash/equality inconsistency for ScalarFunctionExpr
The `ScalarFunctionExpr` equality compares config options ignoring the
order, and so hash should ignore order too.
* Derive dyn eq,hash for ScalarFunctionExpr from plain eq, hash
Use standard pattern for implementing `DynEq` and `DynHash` -- they
should be derived from `Eq` and `Hash` implementations.
* Fix doc example for implementing PhysicalExpr
* Short-circuit expensive operations in eq impl for ScalarFunctionExpr
---
.../physical-expr-common/src/physical_expr.rs | 5 +-
datafusion/physical-expr/src/scalar_function.rs | 76 ++++++++++++----------
2 files changed, 44 insertions(+), 37 deletions(-)
diff --git a/datafusion/physical-expr-common/src/physical_expr.rs b/datafusion/physical-expr-common/src/physical_expr.rs
index 0ab8e56c0b..90c7a1d514 100644
--- a/datafusion/physical-expr-common/src/physical_expr.rs
+++ b/datafusion/physical-expr-common/src/physical_expr.rs
@@ -482,7 +482,7 @@ where
///
/// # Example
/// ```
-/// # // The boiler plate needed to create a `PhysicalExpr` for the example
+/// # // The boilerplate needed to create a `PhysicalExpr` for the example
/// # use std::any::Any;
/// use std::collections::HashMap;
/// # use std::fmt::Formatter;
@@ -492,7 +492,7 @@ where
/// # use datafusion_common::Result;
/// # use datafusion_expr_common::columnar_value::ColumnarValue;
/// # use datafusion_physical_expr_common::physical_expr::{fmt_sql, DynEq, PhysicalExpr};
-/// # #[derive(Debug, Hash, PartialOrd, PartialEq)]
+/// # #[derive(Debug, PartialEq, Eq, Hash)]
/// # struct MyExpr {}
/// # impl PhysicalExpr for MyExpr {fn as_any(&self) -> &dyn Any { unimplemented!() }
/// # fn data_type(&self, input_schema: &Schema) -> Result<DataType> { unimplemented!() }
@@ -504,7 +504,6 @@ where
/// # fn fmt_sql(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "CASE a > b THEN 1 ELSE 0 END") }
/// # }
/// # impl std::fmt::Display for MyExpr {fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { unimplemented!() } }
-/// # impl DynEq for MyExpr {fn dyn_eq(&self, other: &dyn Any) -> bool { unimplemented!() } }
/// # fn make_physical_expr() -> Arc<dyn PhysicalExpr> { Arc::new(MyExpr{}) }
/// let expr: Arc<dyn PhysicalExpr> = make_physical_expr();
/// // wrap the expression in `sql_fmt` which can be used with
diff --git a/datafusion/physical-expr/src/scalar_function.rs b/datafusion/physical-expr/src/scalar_function.rs
index f2bb09b100..eaf1e06156 100644
--- a/datafusion/physical-expr/src/scalar_function.rs
+++ b/datafusion/physical-expr/src/scalar_function.rs
@@ -39,7 +39,7 @@ use crate::PhysicalExpr;
use arrow::array::{Array, RecordBatch};
use arrow::datatypes::{DataType, FieldRef, Schema};
-use datafusion_common::config::ConfigOptions;
+use datafusion_common::config::{ConfigEntry, ConfigOptions};
use datafusion_common::{internal_err, Result, ScalarValue};
use datafusion_expr::interval_arithmetic::Interval;
use datafusion_expr::sort_properties::ExprProperties;
@@ -47,8 +47,6 @@ use datafusion_expr::type_coercion::functions::data_types_with_scalar_udf;
use datafusion_expr::{
expr_vec_fmt, ColumnarValue, ReturnFieldArgs, ScalarFunctionArgs, ScalarUDF,
};
-use datafusion_physical_expr_common::physical_expr::{DynEq, DynHash};
-use itertools::Itertools;
/// Physical expression of a scalar function
pub struct ScalarFunctionExpr {
@@ -175,42 +173,52 @@ impl fmt::Display for ScalarFunctionExpr {
}
}
-impl DynEq for ScalarFunctionExpr {
- fn dyn_eq(&self, other: &dyn Any) -> bool {
- other.downcast_ref::<Self>().is_some_and(|o| {
- self.fun.eq(&o.fun)
- && self.name.eq(&o.name)
- && self.args.eq(&o.args)
- && self.return_field.eq(&o.return_field)
- && self
- .config_options
- .entries()
- .iter()
- .sorted_by(|&l, &r| l.key.cmp(&r.key))
- .zip(
- o.config_options
- .entries()
- .iter()
- .sorted_by(|&l, &r| l.key.cmp(&r.key)),
- )
- .filter(|(l, r)| l.ne(r))
- .count()
- == 0
- })
+impl PartialEq for ScalarFunctionExpr {
+ fn eq(&self, o: &Self) -> bool {
+ if std::ptr::eq(self, o) {
+ // The equality implementation is somewhat expensive, so let's short-circuit when possible.
+ return true;
+ }
+ let Self {
+ fun,
+ name,
+ args,
+ return_field,
+ config_options,
+ } = self;
+ fun.eq(&o.fun)
+ && name.eq(&o.name)
+ && args.eq(&o.args)
+ && return_field.eq(&o.return_field)
+ && (Arc::ptr_eq(config_options, &o.config_options)
+ || sorted_config_entries(config_options)
+ == sorted_config_entries(&o.config_options))
}
}
-
-impl DynHash for ScalarFunctionExpr {
- fn dyn_hash(&self, mut state: &mut dyn Hasher) {
- self.type_id().hash(&mut state);
- self.fun.hash(&mut state);
- self.name.hash(&mut state);
- self.args.hash(&mut state);
- self.return_field.hash(&mut state);
- self.config_options.entries().hash(&mut state);
+impl Eq for ScalarFunctionExpr {}
+impl Hash for ScalarFunctionExpr {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let Self {
+ fun,
+ name,
+ args,
+ return_field,
+ config_options,
+ } = self;
+ fun.hash(state);
+ name.hash(state);
+ args.hash(state);
+ return_field.hash(state);
+ sorted_config_entries(config_options).hash(state);
}
}
+fn sorted_config_entries(config_options: &ConfigOptions) -> Vec<ConfigEntry> {
+ let mut entries = config_options.entries();
+ entries.sort_by(|l, r| l.key.cmp(&r.key));
+ entries
+}
+
impl PhysicalExpr for ScalarFunctionExpr {
/// Return a reference to Any that can be used for downcasting
fn as_any(&self) -> &dyn Any {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org