[GitHub] [calcite] zabetak commented on a change in pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql

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

zabetak commented on a change in pull request #2430:
URL: https://github.com/apache/calcite/pull/2430#discussion_r648254743



##########
File path: core/src/main/java/org/apache/calcite/sql/SqlDialect.java
##########
@@ -1004,6 +1005,13 @@ public boolean supportsGroupByWithCube() {
     return false;
   }
 
+  /**
+   * Returns whether this dialect supports certain type of join in the underlying DBMS.

Review comment:
       Reword: "Returns whether this dialect support the specified type of join."

##########
File path: core/src/main/java/org/apache/calcite/sql/SqlDialect.java
##########
@@ -1004,6 +1005,13 @@ public boolean supportsGroupByWithCube() {
     return false;
   }
 
+  /**
+   * Returns whether this dialect supports certain type of join in the underlying DBMS.
+   */
+  public boolean isSupportedCertainJoinType(JoinRelType certainJoinType) {

Review comment:
       Rename to `supportsJoinType` to be uniform with the other methods in this class and the param to be simply `joinType`.

##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -1045,6 +1045,29 @@ private LockWrapper exclusiveCleanDb(Connection c) throws SQLException {
         .planHasSql("SELECT \"EMPNO\", \"ENAME\"\nFROM \"SCOTT\".\"EMP\"\nWHERE \"EMPNO\" = ?");
   }
 
+  /**
+   * Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-4619">[CALCITE-4619]
+   * "Full join" generates an incorrect execution plan under mysql</a>
+   */
+  @Test void testSupportedCertainJoinType() {
+    CalciteAssert.model(JdbcTest.SCOTT_MYSQL_DIALECT_MODEL)
+        .query("select empno, ename, e.deptno, dname\n"
+            + "from scott.emp e full join scott.dept d\n"
+            + "on e.deptno = d.deptno")
+        .explainContains("PLAN=EnumerableCalc(expr#0..4=[{inputs}], proj#0..2=[{exprs}], " +
+            "DNAME=[$t4])\n" +
+            "  EnumerableHashJoin(condition=[=($2, $3)], joinType=[full])\n" +
+            "    JdbcToEnumerableConverter\n" +
+            "      JdbcProject(EMPNO=[$0], ENAME=[$1], DEPTNO=[$7])\n" +
+            "        JdbcTableScan(table=[[SCOTT, EMP]])\n" +
+            "    JdbcToEnumerableConverter\n" +
+            "      JdbcProject(DEPTNO=[$0], DNAME=[$1])\n" +
+            "        JdbcTableScan(table=[[SCOTT, DEPT]])")
+        .enable(CalciteAssert.DB == CalciteAssert.DatabaseInstance.HSQLDB)
+        .runs();
+  }

Review comment:
       I think it would be better to move the test in `RelToSqlConverterTest`. The class already contains dialect specific tests for MySQL and others and doing so you do not need to add `TestSqlDialectFactoryImpl` class.

##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/H2SqlDialect.java
##########
@@ -40,4 +41,8 @@ public H2SqlDialect(Context context) {
   @Override public boolean supportsWindowFunctions() {
     return false;
   }
+
+  @Override public boolean isSupportedCertainJoinType(JoinRelType certainJoinType) {
+    return certainJoinType != JoinRelType.FULL;
+  }

Review comment:
       Add a test case using H2SqlDialect in `RelToSqlConverterTest`.




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