[GitHub] [calcite] jinxing64 commented on a change in pull request #1555: [CALCITE-3469] Typo in SubstitutionVisitor#rowTypesAreEquivalent

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

jinxing64 commented on a change in pull request #1555: [CALCITE-3469] Typo in SubstitutionVisitor#rowTypesAreEquivalent
URL: https://github.com/apache/calcite/pull/1555#discussion_r341841925
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
 ##########
 @@ -612,7 +612,7 @@ private boolean rowTypesAreEquivalent(
       return litmus.fail("Mismatch for column count: [{}]", Pair.of(rel0, rel1));
     }
     for (Pair<RelDataTypeField, RelDataTypeField> pair
-        : Pair.zip(rel0.rowType.getFieldList(), rel0.rowType.getFieldList())) {
+        : Pair.zip(rel0.rowType.getFieldList(), rel1.rowType.getFieldList())) {
       if (!pair.left.getType().equals(pair.right.getType())) {
         return litmus.fail("Mismatch for column type: [{}]", Pair.of(rel0, rel1));
 
 Review comment:
   Danny~
   We should always have test when proposing change, especially when modifying a function for assertion.
   But it might be hard for this chagne.
   The existing code asserts `rel0` and `rel0` has the equivalent row type by typo mistake. But what it really means is that `rel0` and `rel1` have the equivalent row type, which is guaranteed by matching logic of `SubstitutionVisitor`. As we can see all the tests in MaterializationTest.java passed now.
   What I want to say is that it might be hard to have a light way to construct a test which fails without this PR but passes with this PR.
   But I'm happy if @cndaimin have an good approach to test it.
   

----------------------------------------------------------------
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