You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by jh...@apache.org on 2021/08/06 21:44:12 UTC
[calcite] 01/01: [CALCITE-4485] JDBC adapter generates invalid SQL
when one of the joins is "INNER JOIN ... ON TRUE"
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 <jh...@apache.org>
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() {