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 2021/06/09 11:09:25 UTC
[GitHub] [calcite] angelzouxin opened a new pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql
angelzouxin opened a new pull request #2430:
URL: https://github.com/apache/calcite/pull/2430
https://issues.apache.org/jira/browse/CALCITE-4619
--
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
[GitHub] [calcite] angelzouxin commented on a change in pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql
Posted by GitBox <gi...@apache.org>.
angelzouxin commented on a change in pull request #2430:
URL: https://github.com/apache/calcite/pull/2430#discussion_r648411935
##########
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:
All comments are resolved now.
--
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
[GitHub] [calcite] zabetak commented on a change in pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql
Posted by 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
[GitHub] [calcite] zabetak commented on a change in pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2430:
URL: https://github.com/apache/calcite/pull/2430#discussion_r648390223
##########
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:
You are right `RelToSqlConverterTest` is definitely not the right place. Thinking a bit more, would be sufficient to just change this line:
`.enable(CalciteAssert.DB == CalciteAssert.DatabaseInstance.HSQLDB)`
to
`.enable(CalciteAssert.DB == CalciteAssert.DatabaseInstance.H2 || CalciteAssert.DB == CalciteAssert.DatabaseInstance.MYSQL)`
It is a bit inconvenient since at the moment we are not running with H2 and MySQL in CI but we can verify that it runs locally by using `calcite.test.db` property.
--
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
[GitHub] [calcite] angelzouxin commented on a change in pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql
Posted by GitBox <gi...@apache.org>.
angelzouxin commented on a change in pull request #2430:
URL: https://github.com/apache/calcite/pull/2430#discussion_r648344681
##########
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:
The dialect test in RelToSqlConverterTest does not call the optimizer. Sql.exec method only parses sql into rel and regenerates the sql string according to the configured dialect. I think the joinType test should be placed in JdbcAdapterTest, just like the dialect for supportsWindowFunctions The test case testOverNonSupportedDialect is placed like JdbcAdapterTest. WDYT?
--
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
[GitHub] [calcite] zabetak closed pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2430:
URL: https://github.com/apache/calcite/pull/2430
--
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
[GitHub] [calcite] angelzouxin commented on a change in pull request #2430: [CALCITE-4619] "Full join" generates an incorrect execution plan under mysql
Posted by GitBox <gi...@apache.org>.
angelzouxin commented on a change in pull request #2430:
URL: https://github.com/apache/calcite/pull/2430#discussion_r648344681
##########
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:
The dialect test in RelToSqlConverterTest does not call the optimizer. Sql.exec method only parses sql into rel and regenerates the sql string according to the configured dialect. I think the joinType test should be placed in `JdbcAdapterTest`, just like the `supportsWindowFunctions`.The test case `testOverNonSupportedDialect` is placed in `JdbcAdapterTest`. WDYT?
--
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