You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2019/12/31 08:43:28 UTC

[GitHub] [calcite] chunweilei opened a new pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

chunweilei opened a new pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710
 
 
   JIRA: https://issues.apache.org/jira/browse/CALCITE-2672

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

[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362194242
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
 ##########
 @@ -527,24 +527,48 @@ private static SqlNode expandExprFromJoin(SqlJoin join, SqlIdentifier identifier
   }
 
   private static SqlNode expandCommonColumn(SqlSelect sqlSelect,
-      SqlNode selectItem, SelectScope scope) {
+      SqlNode selectItem, SelectScope scope, SqlValidator validator) {
     if (!(selectItem instanceof SqlIdentifier)) {
       return selectItem;
     }
 
-    final SqlIdentifier identifier = (SqlIdentifier) selectItem;
-    if (!identifier.isSimple()) {
+    final SqlNode from = sqlSelect.getFrom();
+    if (from == null || !(from instanceof SqlJoin)) {
       return selectItem;
     }
 
-    final SqlNode from = sqlSelect.getFrom();
-    if (from == null || !(from instanceof SqlJoin)) {
+    final SqlIdentifier identifier = (SqlIdentifier) selectItem;
+    final SqlConformance conformance = validator.getConformance();
+    if (!identifier.isSimple()) {
+      if (!conformance.allowQualifyingCommonColumn()) {
+        validateQualifiedCommonColumn((SqlJoin) from, identifier, scope, validator);
+      }
       return selectItem;
     }
 
     return expandExprFromJoin((SqlJoin) from, identifier, scope);
   }
 
+  private static void validateQualifiedCommonColumn(SqlJoin join,
+      SqlIdentifier identifier, SelectScope scope, SqlValidator validator) {
+    if (!(join.isNatural() || join.getConditionType() == JoinConditionType.USING)) {
+      return;
+    }
+    for (ScopeChild child : scope.children) {
+      if (child.name.equals(identifier.getComponent(0).toString())) {
 
 Review comment:
   if the column is not a common column ,like `select emp.ename from emp join dept using(deptno)`, it should not fail.

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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362357000
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/materialize/LatticeSuggester.java
 ##########
 @@ -334,7 +334,7 @@ private Lattice findMatch(final Lattice lattice, MutableNode mutableNode) {
       }
 
       if (bestMatch != null) {
-        // Fix up the best batch
+        // Fix up the best match
         for (Path path
 
 Review comment:
   Will do.

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

[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362190668
 
 

 ##########
 File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
 ##########
 @@ -5623,6 +5623,33 @@ private void checkNegWindow(String s, String msg) {
     sql("select * from dept where exists (\n"
         + "select 1 from emp join bonus using (^deptno^))")
         .fails("Column 'DEPTNO' not found in any table");
+
+    // cannot qualify common column in Oracle.
+    sql("select ^emp.deptno^ from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select ^emp.deptno^ from emp natural join dept")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
 
 Review comment:
   duplicate with the case below

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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362171951
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/rex/RexBuilder.java
 ##########
 @@ -669,16 +669,9 @@ private RexNode makeCastIntervalToExact(RelDataType toType, RexNode exp) {
     final TimeUnit endUnit = exp.getType().getSqlTypeName().getEndUnit();
     final TimeUnit baseUnit = baseUnit(exp.getType().getSqlTypeName());
     final BigDecimal multiplier = baseUnit.multiplier;
-    final int scale = 0;
-    BigDecimal divider = endUnit.multiplier.scaleByPowerOfTen(-scale);
+    final BigDecimal divider = endUnit.multiplier;
     RexNode value = multiplyDivide(decodeIntervalOrDecimal(exp),
         multiplier, divider);
-    if (scale > 0) {
-      RelDataType decimalType =
-          typeFactory.createSqlType(SqlTypeName.DECIMAL,
-              scale + exp.getType().getPrecision(), scale);
-      value = encodeIntervalOrDecimal(value, decimalType, false);
-    }
     return ensureType(toType, value, false);
 
 Review comment:
   Dead codes.

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

[GitHub] [calcite] danny0405 commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362182001
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/materialize/LatticeSuggester.java
 ##########
 @@ -334,7 +334,7 @@ private Lattice findMatch(final Lattice lattice, MutableNode mutableNode) {
       }
 
       if (bestMatch != null) {
-        // Fix up the best batch
+        // Fix up the best match
         for (Path path
 
 Review comment:
   There is already a fix there, remove this change or keep credit for the contributor.

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

[GitHub] [calcite] chunweilei merged pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710
 
 
   

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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r363175217
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
 ##########
 @@ -314,4 +314,18 @@ public boolean allowExtendedTrim() {
     }
   }
 
+  @Override public boolean allowQualifyingCommonColumn() {
+    switch (this) {
+    case ORACLE_10:
+    case ORACLE_12:
+    case STRICT_92:
+    case STRICT_99:
+    case STRICT_2003:
+      return false;
 
 Review comment:
   SQL standard also disallows qualifying the common columns[1].
   
   [1] https://dev.mysql.com/doc/refman/8.0/en/join.html

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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362171932
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/rex/RexBuilder.java
 ##########
 @@ -932,13 +925,6 @@ protected RexLiteral makeLiteral(
       }
       break;
     case TIME:
-      assert o instanceof TimeString;
-      p = type.getPrecision();
-      if (p == RelDataType.PRECISION_NOT_SPECIFIED) {
-        p = 0;
-      }
-      o = ((TimeString) o).round(p);
-      break;
     case TIME_WITH_LOCAL_TIME_ZONE:
 
 Review comment:
   These codes are duplicate with TIME_WITH_LOCAL_TIME_ZONE. So remove 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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362357126
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java
 ##########
 @@ -408,4 +408,14 @@
    * false otherwise.
    */
   boolean allowPluralTimeUnits();
+
+  /**
+   * Whether to allow qualifying common column.
+   *
+   * <p>Among the built-in conformance levels, false in
 
 Review comment:
   Will do.

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

[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362192559
 
 

 ##########
 File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
 ##########
 @@ -5623,6 +5623,33 @@ private void checkNegWindow(String s, String msg) {
     sql("select * from dept where exists (\n"
         + "select 1 from emp join bonus using (^deptno^))")
         .fails("Column 'DEPTNO' not found in any table");
+
+    // cannot qualify common column in Oracle.
+    sql("select ^emp.deptno^ from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select ^emp.deptno^ from emp natural join dept")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select ^emp.deptno^ from emp natural join dept")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select count(^emp.deptno^) from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select emp.deptno from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.DEFAULT)
+        .ok();
 
 Review comment:
   can you add case for non common column?

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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362357097
 
 

 ##########
 File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
 ##########
 @@ -5623,6 +5623,33 @@ private void checkNegWindow(String s, String msg) {
     sql("select * from dept where exists (\n"
         + "select 1 from emp join bonus using (^deptno^))")
         .fails("Column 'DEPTNO' not found in any table");
+
+    // cannot qualify common column in Oracle.
+    sql("select ^emp.deptno^ from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select ^emp.deptno^ from emp natural join dept")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select ^emp.deptno^ from emp natural join dept")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select count(^emp.deptno^) from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select emp.deptno from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.DEFAULT)
+        .ok();
 
 Review comment:
   Will do.

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

[GitHub] [calcite] danny0405 commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362181906
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java
 ##########
 @@ -408,4 +408,14 @@
    * false otherwise.
    */
   boolean allowPluralTimeUnits();
+
+  /**
+   * Whether to allow qualifying common column.
+   *
+   * <p>Among the built-in conformance levels, false in
 
 Review comment:
   What do you mean by "common column" ? Can you clarify ?

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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362357024
 
 

 ##########
 File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
 ##########
 @@ -5623,6 +5623,33 @@ private void checkNegWindow(String s, String msg) {
     sql("select * from dept where exists (\n"
         + "select 1 from emp join bonus using (^deptno^))")
         .fails("Column 'DEPTNO' not found in any table");
+
+    // cannot qualify common column in Oracle.
+    sql("select ^emp.deptno^ from emp join dept using (deptno)")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'")
+        .withConformance(SqlConformanceEnum.ORACLE_12)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
+
+    sql("select ^emp.deptno^ from emp natural join dept")
+        .withConformance(SqlConformanceEnum.ORACLE_10)
+        .fails("Cannot qualify common column 'EMP.DEPTNO'");
 
 Review comment:
   Oops, will remove 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

[GitHub] [calcite] chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1710: [CALCITE-2672] Qualifying the common column should not be allowed in Oracle dialect
URL: https://github.com/apache/calcite/pull/1710#discussion_r362357073
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
 ##########
 @@ -527,24 +527,48 @@ private static SqlNode expandExprFromJoin(SqlJoin join, SqlIdentifier identifier
   }
 
   private static SqlNode expandCommonColumn(SqlSelect sqlSelect,
-      SqlNode selectItem, SelectScope scope) {
+      SqlNode selectItem, SelectScope scope, SqlValidator validator) {
     if (!(selectItem instanceof SqlIdentifier)) {
       return selectItem;
     }
 
-    final SqlIdentifier identifier = (SqlIdentifier) selectItem;
-    if (!identifier.isSimple()) {
+    final SqlNode from = sqlSelect.getFrom();
+    if (from == null || !(from instanceof SqlJoin)) {
       return selectItem;
     }
 
-    final SqlNode from = sqlSelect.getFrom();
-    if (from == null || !(from instanceof SqlJoin)) {
+    final SqlIdentifier identifier = (SqlIdentifier) selectItem;
+    final SqlConformance conformance = validator.getConformance();
+    if (!identifier.isSimple()) {
+      if (!conformance.allowQualifyingCommonColumn()) {
+        validateQualifiedCommonColumn((SqlJoin) from, identifier, scope, validator);
+      }
       return selectItem;
     }
 
     return expandExprFromJoin((SqlJoin) from, identifier, scope);
   }
 
+  private static void validateQualifiedCommonColumn(SqlJoin join,
+      SqlIdentifier identifier, SelectScope scope, SqlValidator validator) {
+    if (!(join.isNatural() || join.getConditionType() == JoinConditionType.USING)) {
+      return;
+    }
+    for (ScopeChild child : scope.children) {
+      if (child.name.equals(identifier.getComponent(0).toString())) {
 
 Review comment:
   You're right. But it is surprising that we do not have such test case. I'll add one.

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