[GitHub] [spark] asiunov commented on a diff in pull request #41543: [SPARK-44018][SQL] Improve the hash code for some DS V2 Expression

["asiunov (via GitHub)" <gi...@apache.org>]

asiunov commented on code in PR #41543:
URL: https://github.com/apache/spark/pull/41543#discussion_r1225711782


##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/aggregate/UserDefinedAggregateFunc.java:
##########
@@ -50,4 +53,18 @@ public UserDefinedAggregateFunc(
 
   @Override
   public Expression[] children() { return children; }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
+    UserDefinedAggregateFunc that = (UserDefinedAggregateFunc) o;
+    return Objects.equals(name, that.name) && Objects.equals(canonicalName, that.canonicalName) &&
+      Arrays.equals(children, that.children);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(name, canonicalName) * 31 + Arrays.hashCode(children);

Review Comment:
   ```suggestion
       return Objects.hash(name, canonicalName, isDistinct) * 31 + Arrays.hashCode(children);
   ```



##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/aggregate/GeneralAggregateFunc.java:
##########
@@ -60,4 +63,18 @@ public GeneralAggregateFunc(String name, boolean isDistinct, Expression[] childr
 
   @Override
   public Expression[] children() { return children; }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
+    GeneralAggregateFunc that = (GeneralAggregateFunc) o;
+    return Objects.equals(name, that.name) && Objects.equals(isDistinct, that.isDistinct) &&
+      Arrays.equals(children, that.children);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(name) * 31 + Arrays.hashCode(children);

Review Comment:
   ```suggestion
       return Objects.hash(name, isDistinct) * 31 + Arrays.hashCode(children);
   ```



##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/aggregate/UserDefinedAggregateFunc.java:
##########
@@ -50,4 +53,18 @@ public UserDefinedAggregateFunc(
 
   @Override
   public Expression[] children() { return children; }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
+    UserDefinedAggregateFunc that = (UserDefinedAggregateFunc) o;
+    return Objects.equals(name, that.name) && Objects.equals(canonicalName, that.canonicalName) &&
+      Arrays.equals(children, that.children);

Review Comment:
   (added `isDistinct == that.isDistinct`)
   ```suggestion
       return isDistinct == that.isDistinct && Objects.equals(name, that.name) &&
         Objects.equals(canonicalName, that.canonicalName) && Arrays.equals(children, that.children);
   ```



##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/aggregate/GeneralAggregateFunc.java:
##########
@@ -60,4 +63,18 @@ public GeneralAggregateFunc(String name, boolean isDistinct, Expression[] childr
 
   @Override
   public Expression[] children() { return children; }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
+    GeneralAggregateFunc that = (GeneralAggregateFunc) o;
+    return Objects.equals(name, that.name) && Objects.equals(isDistinct, that.isDistinct) &&

Review Comment:
   bool primitive comparison works faster than `Objects.equals`, also it is usually placed at the beginning, because it is faster to check than string/objects.
   ```suggestion
       return isDistinct == that.isDistinct && Objects.equals(name, that.name) &&
   ```



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org