[GitHub] [calcite] asolimando commented on a diff in pull request #2889: [CALCITE-5253] Natural join fields validation partially broken after CALCITE-5171

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

asolimando commented on code in PR #2889:
URL: https://github.com/apache/calcite/pull/2889#discussion_r984551995


##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6163,7 +6163,7 @@ private ImmutableList<ImmutableBitSet> cube(ImmutableBitSet... sets) {
   /** Test case for
    * <a href="https://issues.apache.org/jira/browse/CALCITE-5171">[CALCITE-5171]
    * NATURAL join and USING should fail if join columns are not unique</a>. */
-  @Test void testNaturalJoinDuplicateColumns() {
+  @Test void testJoinDuplicateColumns() {

Review Comment:
   Whenever possible unit tests should check for a single assertion to scope them more clearly and to easily identify the failure "area" in CI.
   
   What about splitting this single test into multiple tests with proper naming? (you could possible need less comments if the name is clear enough).



##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -3616,9 +3629,20 @@ private RelDataType validateCommonInputJoinColumn(SqlIdentifier id,
     if (field == null) {
       throw newValidationError(id, RESOURCE.columnNotFound(name));
     }
-    if (nameMatcher.frequency(rowType.getFieldNames(), name) > 1) {
-      throw newValidationError(id,
-          RESOURCE.columnInUsingNotUnique(name));
+    Collection<RelDataType> rowTypes;
+    if (!natural && rowType instanceof RelCrossType) {
+      final RelCrossType crossType = (RelCrossType) rowType;
+      ImmutableList<RelDataType> types = crossType.getTypes();
+      rowTypes = new ArrayList<>(types.size());
+      rowTypes.addAll(types);

Review Comment:
   Nitpick: it is equivalent to the shorter version below.
   
   ```suggestion
         rowTypes = new ArrayList<>(crossType.getTypes());
   ```



##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -3616,9 +3629,20 @@ private RelDataType validateCommonInputJoinColumn(SqlIdentifier id,
     if (field == null) {
       throw newValidationError(id, RESOURCE.columnNotFound(name));
     }
-    if (nameMatcher.frequency(rowType.getFieldNames(), name) > 1) {
-      throw newValidationError(id,
-          RESOURCE.columnInUsingNotUnique(name));
+    Collection<RelDataType> rowTypes;
+    if (!natural && rowType instanceof RelCrossType) {
+      final RelCrossType crossType = (RelCrossType) rowType;
+      ImmutableList<RelDataType> types = crossType.getTypes();
+      rowTypes = new ArrayList<>(types.size());
+      rowTypes.addAll(types);
+    } else {
+      rowTypes = Collections.singleton(rowType);
+    }
+    for (RelDataType rowType0 : rowTypes) {
+      if (nameMatcher.frequency(rowType0.getFieldNames(), name) > 1) {
+        throw newValidationError(id,
+            RESOURCE.columnInUsingNotUnique(name));

Review Comment:
   Nitpick: the statement is short enough to fit into one line (similarly to what you have done at line 3630 for a similar case):
    
   ```suggestion
           throw newValidationError(id, RESOURCE.columnInUsingNotUnique(name));
   ```



##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -3586,27 +3587,39 @@ private void validateNoAggs(AggFinder aggFinder, SqlNode node,
     }
   }
 
-  /** Validates a column in a USING clause, or an inferred join key in a
-   * NATURAL join. */
+  /** Validates a column in a USING clause, or an inferred join key in a NATURAL join. */
   private void validateCommonJoinColumn(SqlIdentifier id, SqlNode left,
-      SqlNode right, SqlValidatorScope scope) {
+      SqlNode right, SqlValidatorScope scope, boolean natural) {
     if (id.names.size() != 1) {
       throw newValidationError(id, RESOURCE.columnNotFound(id.toString()));
     }
 
-    final RelDataType leftColType = validateCommonInputJoinColumn(id, left, scope);
-    final RelDataType rightColType = validateCommonInputJoinColumn(id, right, scope);
+    final RelDataType leftColType = natural
+        ? checkAndDeriveDataType(id, left)
+        : validateCommonInputJoinColumn(id, left, scope, natural);
+    final RelDataType rightColType = validateCommonInputJoinColumn(id, right, scope, natural);
     if (!SqlTypeUtil.isComparable(leftColType, rightColType)) {
       throw newValidationError(id,
           RESOURCE.naturalOrUsingColumnNotCompatible(id.getSimple(),
               leftColType.toString(), rightColType.toString()));
     }
   }
 
+  private RelDataType checkAndDeriveDataType(SqlIdentifier id, SqlNode node) {
+    Preconditions.checkArgument(id.names.size() == 1);
+    String name = id.names.get(0);
+    SqlNameMatcher nameMatcher = getCatalogReader().nameMatcher();
+    RelDataType rowType = getNamespaceOrThrow(node).getRowType();
+    RelDataType colType = requireNonNull(
+        nameMatcher.field(rowType, name),
+        () -> "unable to find left field " + name + " in " + rowType).getType();
+    return colType;

Review Comment:
   Nitpick: since modern IDEs allow you to put breakpoints before returning, I generally return immediately without assignment to a temporary variable, but that's a matter of taste I guess, I am fine whichever decision you take.
   
   ```suggestion
       return requireNonNull(
           nameMatcher.field(rowType, name),
           () -> "unable to find left field " + name + " in " + rowType).getType();
   ```



##########
core/src/main/java/org/apache/calcite/rel/type/RelCrossType.java:
##########
@@ -58,6 +58,15 @@ public RelCrossType(
     return false;
   }
 
+  /**
+   * Returns the contained types.
+   *
+   * @return Data types.

Review Comment:
   If you check around, in Calcite it's customary to start with lowercase and omit the full stop at the end for `@return` in Javadoc:
   
   ```suggestion
      * @return data types
   ```



-- 
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@calcite.apache.org

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