alamb commented on code in PR #11392: URL: https://github.com/apache/datafusion/pull/11392#discussion_r1672805356
########## datafusion/expr/src/udaf.rs: ########## @@ -507,6 +510,21 @@ pub trait AggregateUDFImpl: Debug + Send + Sync { fn coerce_types(&self, _arg_types: &[DataType]) -> Result<Vec<DataType>> { not_impl_err!("Function {} does not implement coerce_types", self.name()) } + + /// Dynamic equality. Allows customizing the equality of aggregate UDFs. + /// By default, compares the UDF name and signature. Review Comment: Here is a suggestion to improve the docstring ```suggestion /// Return true if this aggregate UDF is equal to the other. /// /// Allows customizing the equality of aggregate UDFs. Must be consistent /// with [`Self::hash_value`]. /// /// By default, compares [`Self::name`] and [`Self::signature`] ``` ########## datafusion/expr/src/udf.rs: ########## @@ -540,6 +541,21 @@ pub trait ScalarUDFImpl: Debug + Send + Sync { fn coerce_types(&self, _arg_types: &[DataType]) -> Result<Vec<DataType>> { not_impl_err!("Function {} does not implement coerce_types", self.name()) } + + /// Dynamic equality. Allows customizing the equality of scalar UDFs. Review Comment: I recommend the same documentation updates here as for aggregateUDF ########## datafusion/expr/src/udaf.rs: ########## @@ -562,6 +580,18 @@ impl AggregateUDFImpl for AliasedAggregateUDFImpl { fn aliases(&self) -> &[String] { &self.aliases } + + fn equals(&self, other: &dyn AggregateUDFImpl) -> bool { Review Comment: this makes sense to me as the name and signature are the same as the inner ########## datafusion/expr/src/udaf.rs: ########## @@ -72,20 +76,19 @@ pub struct AggregateUDF { impl PartialEq for AggregateUDF { fn eq(&self, other: &Self) -> bool { - self.name() == other.name() && self.signature() == other.signature() + self.inner.equals(other.inner.as_ref()) || other.inner.equals(self.inner.as_ref()) Review Comment: Another possibility we could do is document that the equality test in the UDFs must be symmetric. ########## datafusion/expr/src/udaf.rs: ########## @@ -507,6 +510,21 @@ pub trait AggregateUDFImpl: Debug + Send + Sync { fn coerce_types(&self, _arg_types: &[DataType]) -> Result<Vec<DataType>> { not_impl_err!("Function {} does not implement coerce_types", self.name()) } + + /// Dynamic equality. Allows customizing the equality of aggregate UDFs. + /// By default, compares the UDF name and signature. + fn equals(&self, other: &dyn AggregateUDFImpl) -> bool { + self.name() == other.name() && self.signature() == other.signature() + } + + /// Dynamic hashing. Allows customizing the hash code of aggregate UDFs. + /// By default, hashes the UDF name and signature. + fn hash_value(&self) -> u64 { Review Comment: I think it would be good to note here that eq and hash value need to be consistent. Something like this perhaps: ```suggestion /// Returns a hash value for this aggregate UDF. /// /// Allows customizing the hash code of aggregate UDFs. Similarly to /// [`std::hash::Hash`], if [`Self::equals`] /// returns true for two aggregate UDFs, the value of `hash_value` must as well. /// /// By default, hashes [`Self::name`] and [`Self::signature`] fn hash_value(&self) -> u64 { ``` ########## datafusion/expr/src/udwf.rs: ########## @@ -296,6 +299,21 @@ pub trait WindowUDFImpl: Debug + Send + Sync { fn simplify(&self) -> Option<WindowFunctionSimplification> { None } + + /// Dynamic equality. Allows customizing the equality of window UDFs. Review Comment: Likewise here for doc updates -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org