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

Reply via email to