This is an automated email from the ASF dual-hosted git repository. jhyde pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 8346167252bcb90cd64ced9e3a5c893de28f88ed Author: Julian Hyde <[email protected]> AuthorDate: Tue Aug 3 21:36:20 2021 -0700 [CALCITE-4485] JDBC adapter generates invalid SQL when one of the joins is "INNER JOIN ... ON TRUE" If the join tree is a mixture of cross-joins and outer joins, RelToSqlConverter wrongly generates comma-join syntax. Consider the (pseudo) RelNode tree CrossJoin(a, LeftJoin(b, CrossJoin(c, d))) Before this bug was fixed, RelToSqlConvert would generate FROM a, b LEFT JOIN c, d Because LEFT JOIN has higher precedence than ',', this is parsed to LeftJoin(CrossJoin(a, b), CrossJoin(c, d)) which is incorrect. The fix is to only generate comma-join syntax if all joins are cross-joins. In this case, we will generate (pseudo SQL) FROM a CROSS JOIN b LEFT JOIN c CROSS JOIN d which is safe. --- .../calcite/rel/rel2sql/RelToSqlConverter.java | 100 +++++++++++++-- .../calcite/rel/rel2sql/RelToSqlConverterTest.java | 140 ++++++++++++++++++++- 2 files changed, 229 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java b/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java index c9fb0eb..4377e84 100644 --- a/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java +++ b/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java @@ -217,16 +217,22 @@ public class RelToSqlConverter extends SqlImplementor final Context leftContext = leftResult.qualifiedContext(); final Context rightContext = rightResult.qualifiedContext(); final SqlNode sqlCondition; - SqlLiteral condType = JoinConditionType.ON.symbol(POS); + final JoinConditionType condType; JoinType joinType = joinType(e.getJoinType()); - if (isCrossJoin(e)) { + if (joinType == JoinType.INNER + && e.getCondition().isAlwaysTrue()) { + if (isCommaJoin(e)) { + joinType = JoinType.COMMA; + } else { + joinType = JoinType.CROSS; + } sqlCondition = null; - joinType = dialect.emulateJoinTypeForCrossJoin(); - condType = JoinConditionType.NONE.symbol(POS); + condType = JoinConditionType.NONE; } else { sqlCondition = convertConditionToSqlNode(e.getCondition(), leftContext, rightContext); + condType = JoinConditionType.ON; } SqlNode join = new SqlJoin(POS, @@ -234,7 +240,7 @@ public class RelToSqlConverter extends SqlImplementor SqlLiteral.createBoolean(false, POS), joinType.symbol(POS), rightResult.asFrom(), - condType, + condType.symbol(POS), sqlCondition); return result(join, leftResult, rightResult); } @@ -289,8 +295,88 @@ public class RelToSqlConverter extends SqlImplementor return result(resultNode, leftResult, rightResult); } - private static boolean isCrossJoin(final Join e) { - return e.getJoinType() == JoinRelType.INNER && e.getCondition().isAlwaysTrue(); + /** Returns whether this join should be unparsed as a {@link JoinType#COMMA}. + * + * <p>Comma-join is one possible syntax for {@code CROSS JOIN ... ON TRUE}, + * supported on most but not all databases + * (see {@link SqlDialect#emulateJoinTypeForCrossJoin()}). + * + * <p>For example, the following queries are equivalent: + * + * <pre>{@code + * // Comma join + * SELECT * + * FROM Emp, Dept + * + * // Cross join + * SELECT * + * FROM Emp CROSS JOIN Dept + * + * // Inner join + * SELECT * + * FROM Emp INNER JOIN Dept ON TRUE + * }</pre> + * + * <p>Examples: + * <ul> + * <li>{@code FROM (x CROSS JOIN y ON TRUE) CROSS JOIN z ON TRUE} + * is a comma join, because all joins are comma joins; + * <li>{@code FROM (x CROSS JOIN y ON TRUE) CROSS JOIN z ON TRUE} + * would not be a comma join when run on Apache Spark, because Spark does + * not support comma join; + * <li>{@code FROM (x CROSS JOIN y ON TRUE) LEFT JOIN z ON TRUE} + * is not comma join because one of the joins is not INNER; + * <li>{@code FROM (x CROSS JOIN y ON c) CROSS JOIN z ON TRUE} + * is not a comma join because one of the joins is ON TRUE. + * </ul> + */ + private boolean isCommaJoin(Join join) { + if (!join.getCondition().isAlwaysTrue()) { + return false; + } + if (dialect.emulateJoinTypeForCrossJoin() != JoinType.COMMA) { + return false; + } + + // Find the topmost enclosing Join. For example, if the tree is + // Join((Join(a, b), Join(c, Join(d, e))) + // we get the same topJoin for a, b, c, d and e. Join. Stack is never empty, + // and frame.r on the first iteration is always "join". + assert !stack.isEmpty(); + assert stack.element().r == join; + Join j = null; + for (Frame frame : stack) { + j = (Join) frame.r; + if (!(frame.parent instanceof Join)) { + break; + } + } + final Join topJoin = requireNonNull(j, "top join"); + + // Flatten the join tree, using a breadth-first algorithm. + // After flattening, the list contains all of the joins that will make up + // the FROM clause. + final List<Join> flatJoins = new ArrayList<>(); + flatJoins.add(topJoin); + for (int i = 0; i < flatJoins.size();) { + final Join j2 = flatJoins.get(i++); + if (j2.getLeft() instanceof Join) { + flatJoins.add((Join) j2.getLeft()); + } + if (j2.getRight() instanceof Join) { + flatJoins.add((Join) j2.getRight()); + } + } + + // If all joins are cross-joins (INNER JOIN ON TRUE), we can use + // we can use comma syntax "FROM a, b, c, d, e". + for (Join j2 : flatJoins) { + if (j2.getJoinType() != JoinRelType.INNER + || !j2.getCondition().isAlwaysTrue()) { + return false; + } + } + return true; } /** Visits a Correlate; called by {@link #dispatch} via reflection. */ diff --git a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java index b41c9ad..51ed1f3 100644 --- a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java +++ b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java @@ -92,6 +92,7 @@ import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.UnaryOperator; import java.util.stream.Collectors; @@ -5255,12 +5256,143 @@ class RelToSqlConverterTest { sql(query).ok(expected); } - @Test void testCrossJoinEmulationForSpark() { - String query = "select * from \"employee\", \"department\""; - final String expected = "SELECT *\n" + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-4485";>[CALCITE-4485] + * JDBC adapter generates invalid SQL when one of the joins is {@code INNER + * JOIN ... ON TRUE}</a>. */ + @Test void testCommaCrossJoin() { + final Function<RelBuilder, RelNode> relFn = b -> + b.scan("tpch", "customer") + .aggregate(b.groupKey(b.field("nation_name")), + b.count().as("cnt1")) + .project(b.field("nation_name"), b.field("cnt1")) + .as("cust") + .scan("tpch", "lineitem") + .aggregate(b.groupKey(), + b.count().as("cnt2")) + .project(b.field("cnt2")) + .as("lineitem") + .join(JoinRelType.INNER) + .scan("tpch", "part") + .join(JoinRelType.LEFT, + b.equals(b.field(2, "cust", "nation_name"), + b.field(2, "part", "p_brand"))) + .project(b.field("cust", "nation_name"), + b.alias( + b.call(SqlStdOperatorTable.MINUS, + b.field("cnt1"), + b.field("cnt2")), + "f1")) + .build(); + + // For documentation purposes, here is the query that was generated before + // [CALCITE-4485] was fixed. + final String previousPostgresql = "" + + "SELECT \"t\".\"nation_name\", \"t\".\"cnt1\" - \"t0\".\"cnt2\" AS \"f1\"\n" + + "FROM (SELECT \"nation_name\", COUNT(*) AS \"cnt1\"\n" + + "FROM \"tpch\".\"customer\"\n" + + "GROUP BY \"nation_name\") AS \"t\",\n" + + "(SELECT COUNT(*) AS \"cnt2\"\n" + + "FROM \"tpch\".\"lineitem\") AS \"t0\"\n" + + "LEFT JOIN \"tpch\".\"part\" ON \"t\".\"nation_name\" = \"part\".\"p_brand\""; + final String expectedPostgresql = "" + + "SELECT \"t\".\"nation_name\", \"t\".\"cnt1\" - \"t0\".\"cnt2\" AS \"f1\"\n" + + "FROM (SELECT \"nation_name\", COUNT(*) AS \"cnt1\"\n" + + "FROM \"tpch\".\"customer\"\n" + + "GROUP BY \"nation_name\") AS \"t\"\n" + + "CROSS JOIN (SELECT COUNT(*) AS \"cnt2\"\n" + + "FROM \"tpch\".\"lineitem\") AS \"t0\"\n" + + "LEFT JOIN \"tpch\".\"part\" ON \"t\".\"nation_name\" = \"part\".\"p_brand\""; + relFn(relFn) + .schema(CalciteAssert.SchemaSpec.TPCH) + .withPostgresql() + .ok(expectedPostgresql); + } + + /** A cartesian product is unparsed as a CROSS JOIN on Spark, + * comma join on other DBs. + * + * @see SqlDialect#emulateJoinTypeForCrossJoin() + */ + @Test void testCrossJoinEmulation() { + final String expectedSpark = "SELECT *\n" + "FROM foodmart.employee\n" + "CROSS JOIN foodmart.department"; - sql(query).withSpark().ok(expected); + final String expectedMysql = "SELECT *\n" + + "FROM `foodmart`.`employee`,\n" + + "`foodmart`.`department`"; + Consumer<String> fn = sql -> + sql(sql) + .withSpark().ok(expectedSpark) + .withMysql().ok(expectedMysql); + fn.accept("select * from \"employee\", \"department\""); + fn.accept("select * from \"employee\" cross join \"department\""); + fn.accept("select * from \"employee\" join \"department\" on true"); + } + + /** Similar to {@link #testCommaCrossJoin()} (but uses SQL) + * and {@link #testCrossJoinEmulation()} (but is 3 way). We generate a comma + * join if the only joins are {@code CROSS JOIN} or + * {@code INNER JOIN ... ON TRUE}, and if we're not on Spark. */ + @Test void testCommaCrossJoin3way() { + String sql = "select *\n" + + "from \"store\" as s\n" + + "inner join \"employee\" as e on true\n" + + "cross join \"department\" as d"; + final String expectedMysql = "SELECT *\n" + + "FROM `foodmart`.`store`,\n" + + "`foodmart`.`employee`,\n" + + "`foodmart`.`department`"; + final String expectedSpark = "SELECT *\n" + + "FROM foodmart.store\n" + + "CROSS JOIN foodmart.employee\n" + + "CROSS JOIN foodmart.department"; + sql(sql) + .withMysql().ok(expectedMysql) + .withSpark().ok(expectedSpark); + } + + /** As {@link #testCommaCrossJoin3way()}, but shows that if there is a + * {@code LEFT JOIN} in the FROM clause, we can't use comma-join. */ + @Test void testLeftJoinPreventsCommaJoin() { + String sql = "select *\n" + + "from \"store\" as s\n" + + "left join \"employee\" as e on true\n" + + "cross join \"department\" as d"; + final String expectedMysql = "SELECT *\n" + + "FROM `foodmart`.`store`\n" + + "LEFT JOIN `foodmart`.`employee` ON TRUE\n" + + "CROSS JOIN `foodmart`.`department`"; + sql(sql).withMysql().ok(expectedMysql); + } + + /** As {@link #testLeftJoinPreventsCommaJoin()}, but the non-cross-join + * occurs later in the FROM clause. */ + @Test void testRightJoinPreventsCommaJoin() { + String sql = "select *\n" + + "from \"store\" as s\n" + + "cross join \"employee\" as e\n" + + "right join \"department\" as d on true"; + final String expectedMysql = "SELECT *\n" + + "FROM `foodmart`.`store`\n" + + "CROSS JOIN `foodmart`.`employee`\n" + + "RIGHT JOIN `foodmart`.`department` ON TRUE"; + sql(sql).withMysql().ok(expectedMysql); + } + + /** As {@link #testLeftJoinPreventsCommaJoin()}, but the impediment is a + * {@code JOIN} whose condition is not {@code TRUE}. */ + @Test void testOnConditionPreventsCommaJoin() { + String sql = "select *\n" + + "from \"store\" as s\n" + + "join \"employee\" as e on s.\"store_id\" = e.\"store_id\"\n" + + "cross join \"department\" as d"; + final String expectedMysql = "SELECT *\n" + + "FROM `foodmart`.`store`\n" + + "INNER JOIN `foodmart`.`employee`" + + " ON `store`.`store_id` = `employee`.`store_id`\n" + + "CROSS JOIN `foodmart`.`department`"; + sql(sql).withMysql().ok(expectedMysql); } @Test void testSubstringInSpark() {
