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:
[email protected]
With regards,
Apache Git Services
