[GitHub] [doris] englefly commented on a diff in pull request #11067: [feature](nereids): add equals for expression

[GitBox <gi...@apache.org>]

englefly commented on code in PR #11067:
URL: https://github.com/apache/doris/pull/11067#discussion_r926816905


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Alias.java:
##########
@@ -116,14 +115,13 @@ public String toString() {
         return child().toString() + " AS `" + name + "`#" + exprId;
     }
 
-    public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
-        return visitor.visitAlias(this, context);
-    }
-
     @Override
     public Expression withChildren(List<Expression> children) {
         Preconditions.checkArgument(children.size() == 1);
         return new Alias(exprId, children.get(0), name);
     }
 
+    public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {

Review Comment:
   why just move this function's position?



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java:
##########
@@ -72,4 +74,19 @@ public static DataType convertFromCatalogDataType(Type catalogType) {
 
     public abstract Type toCatalogDataType();
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
+        return true;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash();

Review Comment:
   it is better to return 0 directly. Objects.hash() will return 0.



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Alias.java:
##########
@@ -98,12 +98,11 @@ public boolean equals(Object o) {
         if (o == null || getClass() != o.getClass()) {
             return false;
         }
-        if (!super.equals(o)) {
-            return false;
-        }
-        Alias alias = (Alias) o;
-        return exprId.equals(alias.exprId) && name.equals(alias.name)
-                && qualifier.equals(alias.qualifier) && children.equals(alias.children);
+        Alias that = (Alias) o;
+        return exprId.equals(that.exprId)

Review Comment:
   how about remove "exprId.equals(that.exprId)" ?
   do you have an example to illustrate its necessity?



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/VarcharType.java:
##########
@@ -38,4 +40,18 @@ public static VarcharType createVarcharType(int len) {
     public Type toCatalogDataType() {
         return ScalarType.createVarcharType(len);
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (!super.equals(o)) {
+            return false;
+        }
+        VarcharType that = (VarcharType) o;

Review Comment:
   this may throw java.lang.ClassCastException. It is better to compare their class before convert.



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ComparisonPredicate.java:
##########
@@ -77,7 +77,7 @@ public boolean equals(Object o) {
             return false;
         }
         ComparisonPredicate other = (ComparisonPredicate) o;
-        return (type == other.getType()) && Objects.equals(left(), other.left())
+        return Objects.equals(left(), other.left())

Review Comment:
   Could you explain why we do not compare type?



-- 
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: commits-unsubscribe@doris.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org