[GitHub] [calcite] zabetak commented on a change in pull request #1814: [CALCITE-3804] Use RelCollation interface in RelCollationImpl compareTo and satisfies methods

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

zabetak commented on a change in pull request #1814: [CALCITE-3804] Use RelCollation interface in RelCollationImpl compareTo and satisfies methods
URL: https://github.com/apache/calcite/pull/1814#discussion_r380705602
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/rel/RelCollationImpl.java
 ##########
 @@ -97,9 +96,11 @@ public boolean isTop() {
   }
 
   public int compareTo(@Nonnull RelMultipleTrait o) {
-    final RelCollationImpl that = (RelCollationImpl) o;
-    final UnmodifiableIterator<RelFieldCollation> iterator =
-        that.fieldCollations.iterator();
+    if (this == o) {
+      return 0;
+    }
+    final RelCollation that = (RelCollation) o;
 
 Review comment:
   Allowing comparisons between instances of different classes seems a bit risky. If somebody creates a new class `RelCollationImpl2` and defines another method `compareTo` then it is not easy/possible to guarantee that the comparison between `x instanceof RelCollationImpl` and `y instanceof RelCollationImpl2` (i.e., `sgn(x.compareTo(y)) ==-sgn(y.compareTo(x)`) is for instance symmetric. Can you elaborate why is it necessary to support this behavior?
   
   If we want to allow comparisons between instances of different classes (under the same interface `RelCollation`) then it is more appropriate to make this method default and move it to the interface with adequate documentation. 
   
   The same comment I think applies for the `satisfies` method.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services