[CALCITE-6266] SqlValidatorException with LATERAL TABLE and JOIN

The current precedence of ',' (the comma operator in
'FROM t1, t2') and 'JOIN' is wrong. JOIN is currently higher,
and so in a query like

  SELECT *
  FROM dept,
    LATERAL (SELECT * FROM emp WHERE emp.deptno = dept.deptno) AS emp
    CROSS JOIN (VALUES ('A'), ('B')) AS v (v);

the emp subqery is associated with v more tightly than dept,
and therefore dept.deptno is not visible; the correct
association is not (dept (emp v)) but ((dept emp) v).

The LATERAL keyword, and the TABLE function mentioned in the
bug, are not the root cause, but they do make the problem
obvious because the wrong association causes a validation
error.

This commit fixes the precedence of the operators.

Close apache#3871

core/src/main/codegen/templates/Parser.jj

@@ -2037,7 +2037,7 @@ SqlNode FromClause() :
     SqlLiteral joinType;
 }
 {
-    e = Join()
+    e = TableRef1(ExprContext.ACCEPT_QUERY_OR_JOIN)
     (
         // Comma joins should only occur at top-level in the FROM clause.
         // Valid:
@@ -2046,33 +2046,30 @@ SqlNode FromClause() :
         // Not valid:
         //  * FROM a CROSS JOIN (b, c)
         LOOKAHEAD(1)
-        <COMMA> { joinType = JoinType.COMMA.symbol(getPos()); }
-        e2 = Join() {
-            e = new SqlJoin(joinType.getParserPosition(),
-                e,
-                SqlLiteral.createBoolean(false, joinType.getParserPosition()),
-                joinType,
-                e2,
-                JoinConditionType.NONE.symbol(SqlParserPos.ZERO),
-                null);
-        }
+        e = JoinOrCommaTable(e)
     )*
     { return e; }
 }
 
-SqlNode Join() :
+SqlNode JoinOrCommaTable(SqlNode e) :
 {
-    SqlNode e;
+    SqlNode e2;
+    SqlLiteral joinType;
 }
 {
-    e = TableRef1(ExprContext.ACCEPT_QUERY_OR_JOIN)
-    (
-        LOOKAHEAD(2)
-        e = JoinTable(e)
-    )*
-    {
-        return e;
+    LOOKAHEAD(2)
+    <COMMA> { joinType = JoinType.COMMA.symbol(getPos()); }
+    e2 = TableRef1(ExprContext.ACCEPT_QUERY_OR_JOIN) {
+        return new SqlJoin(joinType.getParserPosition(),
+            e,
+            SqlLiteral.createBoolean(false, joinType.getParserPosition()),
+            joinType,
+            e2,
+            JoinConditionType.NONE.symbol(SqlParserPos.ZERO),
+            null);
     }
+|
+    e2 = JoinTable(e) { return e2; }
 }
 
 /** Matches "LEFT JOIN t ON ...", "RIGHT JOIN t USING ...", "JOIN t". */
@@ -2228,6 +2225,8 @@ SqlNode TableRef3(ExprContext exprContext, boolean lateral) :
         tableRef = addLateral(tableRef, lateral)
         [ tableRef = MatchRecognize(tableRef) ]
     |
+        LOOKAHEAD(2)
+        [ <LATERAL> ] // "LATERAL" is implicit with "UNNEST", so ignore
         <UNNEST> { s = span(); }
         args = ParenthesizedQueryOrCommaList(ExprContext.ACCEPT_SUB_QUERY)
         [

core/src/main/java/org/apache/calcite/sql/SqlJoin.java

@@ -36,7 +36,7 @@
  */
 public class SqlJoin extends SqlCall {
   static final SqlJoinOperator COMMA_OPERATOR =
-      new SqlJoinOperator("COMMA-JOIN", 16);
+      new SqlJoinOperator("COMMA-JOIN", 18);
   public static final SqlJoinOperator OPERATOR =
       new SqlJoinOperator("JOIN", 18);
 

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java

@@ -8238,6 +8238,38 @@ void testGroupExpressionEquivalenceParams() {
         .fails("Table 'DEPT' not found");
   }
 
+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-6266">[CALCITE-6266]
+   * SqlValidatorException with LATERAL TABLE and JOIN</a>. */
+  @Test void testCollectionTableWithLateral3() {
+    // The cause of [CALCITE-6266] was that CROSS JOIN had higher precedence
+    // than ",", and therefore "table(ramp(deptno)" was being associated with
+    // "values".
+    sql("select *\n"
+        + "from dept,\n"
+        + "  lateral table(ramp(deptno))\n"
+        + "  cross join (values ('A'), ('B'))")
+        .ok();
+    // As above, using NATURAL JOIN
+    sql("select *\n"
+        + "from dept,\n"
+        + "  lateral table(ramp(deptno))\n"
+        + "  natural join emp")
+        .ok();
+    // As above, using comma
+    sql("select *\n"
+        + "from emp,\n"
+        + "  lateral (select * from dept where dept.deptno = emp.deptno),\n"
+        + "  emp as e2")
+        .ok();
+    // Without 'as e2', relation name is not unique
+    sql("select *\n"
+        + "from emp,\n"
+        + "  lateral (select * from dept where dept.deptno = emp.deptno),\n"
+        + "  ^emp^")
+        .fails("Duplicate relation name 'EMP' in FROM clause");
+  }
+
   @Test void testCollectionTableWithCursorParam() {
     sql("select * from table(dedup(cursor(select * from emp),'ename'))")
         .type("RecordType(VARCHAR(1024) NOT NULL NAME) NOT NULL");

core/src/test/resources/sql/lateral.iq

@@ -136,4 +136,81 @@ from "scott".dept,
 
 !ok
 
+# LATERAL plus CTEs (WITH)
+with dept (deptno, dname)
+    as (values (10, 'ACCOUNTING'), (20, 'RESEARCH')),
+  emp (empno, deptno, ename)
+    as (values (7369, 20, 'SMITH'), (7782, 10, 'CLARK'), (7499, 30, 'ALLEN'))
+select *
+from dept,
+  lateral (select * from emp where emp.deptno = dept.deptno) as emp;
++--------+------------+-------+---------+-------+
+| DEPTNO | DNAME      | EMPNO | DEPTNO0 | ENAME |
++--------+------------+-------+---------+-------+
+|     10 | ACCOUNTING |  7782 |      10 | CLARK |
+|     20 | RESEARCH   |  7369 |      20 | SMITH |
++--------+------------+-------+---------+-------+
+(2 rows)
+
+!ok
+
+# [CALCITE-6266] SqlValidatorException with LATERAL TABLE and JOIN
+# The problem also occurred in ', LATERAL (SELECT ...) JOIN', because
+# ',' should had lower precedence than 'JOIN', but should have had equal
+# precedence.
+with dept (deptno, dname)
+    as (values (10, 'ACCOUNTING'), (20,'RESEARCH')),
+  emp (empno, deptno, ename)
+    as (values (7369, 20, 'SMITH'), (7782, 10, 'CLARK'), (7499, 30, 'ALLEN'))
+select *
+from dept,
+  lateral (select * from emp where emp.deptno = dept.deptno) as emp
+  cross join (values 'A', 'B') as v (v);
++--------+------------+-------+---------+-------+---+
+| DEPTNO | DNAME      | EMPNO | DEPTNO0 | ENAME | V |
++--------+------------+-------+---------+-------+---+
+|     10 | ACCOUNTING |  7782 |      10 | CLARK | A |
+|     10 | ACCOUNTING |  7782 |      10 | CLARK | B |
+|     20 | RESEARCH   |  7369 |      20 | SMITH | A |
+|     20 | RESEARCH   |  7369 |      20 | SMITH | B |
++--------+------------+-------+---------+-------+---+
+(4 rows)
+
+!ok
+
+# UNNEST applied to VALUES
+with t (x, ys)
+  as (values (1, array [2,3]), (4, array [5]))
+select *
+from t,
+  unnest(t.ys) as y;
++---+--------+---+
+| X | YS     | Y |
++---+--------+---+
+| 1 | [2, 3] | 2 |
+| 1 | [2, 3] | 3 |
+| 4 | [5]    | 5 |
++---+--------+---+
+(3 rows)
+
+!ok
+
+# LATERAL UNNEST means the same as UNNEST
+# (Have checked Postgres)
+with t (x, ys)
+  as (values (1, array [2,3]), (4, array [5]))
+select *
+from t,
+  lateral unnest(t.ys) as y;
++---+--------+---+
+| X | YS     | Y |
++---+--------+---+
+| 1 | [2, 3] | 2 |
+| 1 | [2, 3] | 3 |
+| 4 | [5]    | 5 |
++---+--------+---+
+(3 rows)
+
+!ok
+
 # End lateral.iq

testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java

@@ -20,6 +20,7 @@
 import org.apache.calcite.sql.SqlDialect;
 import org.apache.calcite.sql.SqlExplain;
 import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlJoin;
 import org.apache.calcite.sql.SqlKind;
 import org.apache.calcite.sql.SqlLambda;
 import org.apache.calcite.sql.SqlLiteral;
@@ -75,6 +76,7 @@
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.notNullValue;
@@ -86,6 +88,8 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assumptions.assumeFalse;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * A <code>SqlParserTest</code> is a unit-test for
  * {@link SqlParser the SQL parser}.
@@ -676,6 +680,19 @@ private static Matcher<SqlNode> isQuoted(final int i,
     };
   }
 
+  /** Returns a {@link Matcher} that calls a consumer and then succeeds.
+   * The consumer should contain custom code, and should fail if it doesn't
+   * like what it sees. */
+  public static Matcher<SqlNode> customMatches(String description,
+      Consumer<SqlNode> consumer) {
+    return new CustomTypeSafeMatcher<SqlNode>(description) {
+      @Override protected boolean matchesSafely(SqlNode sqlNode) {
+        consumer.accept(sqlNode);
+        return true;
+      }
+    };
+  }
+
   protected SortedSet<String> getReservedKeywords() {
     return keywords("c");
   }
@@ -3203,6 +3220,28 @@ void checkPeriodPredicate(Checker checker) {
             + "CROSS JOIN `B`");
   }
 
+  @Test void testJoinCrossComma() {
+    sql("select * from a as a2, b cross join c")
+        .node(
+            customMatches("custom", node -> {
+              // Parsed as left-deep:
+              //   select * from (a as a2, b) cross join c
+              // (This is not valid SQL, but illustrates operator
+              // associativity.)
+              assertThat(node, instanceOf(SqlSelect.class));
+              final SqlSelect select = (SqlSelect) node;
+              assertThat(select.getFrom(), instanceOf(SqlJoin.class));
+              final SqlJoin from = requireNonNull((SqlJoin) select.getFrom());
+              assertThat(from.getLeft(), instanceOf(SqlJoin.class));
+              assertThat(from.getRight(), instanceOf(SqlIdentifier.class));
+            }));
+  }
+
+  @Test void testInternalComma() {
+    sql("select * from (a^,^ b) cross join c")
+        .fails("(?s)Encountered \",\" at .*");
+  }
+
   @Test void testJoinOn() {
     sql("select * from a left join b on 1 = 1 and 2 = 2 where 3 = 3")
         .ok("SELECT *\n"
@@ -4362,15 +4401,15 @@ void checkPeriodPredicate(Checker checker) {
             + "Was expecting one of:\n"
             + "    \"LATERAL\" \\.\\.\\.\n"
             + "    \"TABLE\" \\.\\.\\.\n"
-            + "    \"UNNEST\" \\.\\.\\.\n"
             + "    <IDENTIFIER> \\.\\.\\.\n"
             + "    <HYPHENATED_IDENTIFIER> \\.\\.\\.\n"
             + "    <QUOTED_IDENTIFIER> \\.\\.\\.\n"
             + "    <BACK_QUOTED_IDENTIFIER> \\.\\.\\.\n"
             + "    <BIG_QUERY_BACK_QUOTED_IDENTIFIER> \\.\\.\\.\n"
             + "    <BRACKET_QUOTED_IDENTIFIER> \\.\\.\\.\n"
             + "    <UNICODE_QUOTED_IDENTIFIER> \\.\\.\\.\n"
-            + "    \"\\(\" \\.\\.\\.\n.*");
+            + "    \"\\(\" \\.\\.\\.\n.*"
+            + "    \"UNNEST\" \\.\\.\\.\n.*");
   }
 
   @Test void testEmptyValues() {
@@ -6061,8 +6100,8 @@ private static Matcher<SqlNode> isCharLiteral(String s) {
             + "FROM (VALUES (ROW(1))) AS `X`)))");
     sql("SELECT multiset(SELECT x FROM (VALUES(1)) x ^ORDER^ BY x)")
         .fails("(?s)Encountered \"ORDER\" at.*");
-    sql("SELECT multiset(SELECT x FROM (VALUES(1)) x, ^SELECT^ x FROM (VALUES(1)) x)")
-        .fails("(?s)Incorrect syntax near the keyword 'SELECT' at.*");
+    sql("SELECT multiset(SELECT x FROM (VALUES(1)) x^,^ SELECT x FROM (VALUES(1)) x)")
+        .fails("(?s)Encountered \", SELECT\" at.*");
     sql("SELECT multiset(^1^, SELECT x FROM (VALUES(1)) x)")
         .fails("(?s)Non-query expression encountered in illegal context");
   }
@@ -6169,8 +6208,8 @@ private static Matcher<SqlNode> isCharLiteral(String s) {
         .ok("SELECT (ARRAY (SELECT `X`\n"
             + "FROM (VALUES (ROW(1))) AS `X`\n"
             + "ORDER BY `X`))");
-    sql("SELECT array(SELECT x FROM (VALUES(1)) x, ^SELECT^ x FROM (VALUES(1)) x)")
-      .fails("(?s)Incorrect syntax near the keyword 'SELECT' at.*");
+    sql("SELECT array(SELECT x FROM (VALUES(1)) x^,^ SELECT x FROM (VALUES(1)) x)")
+      .fails("(?s)Encountered \", SELECT\" at.*");
     sql("SELECT array(1, ^SELECT^ x FROM (VALUES(1)) x)")
       .fails("(?s)Incorrect syntax near the keyword 'SELECT'.*");
   }
@@ -6309,8 +6348,8 @@ private static Matcher<SqlNode> isCharLiteral(String s) {
 
     sql("SELECT map(1, ^SELECT^ x FROM (VALUES(1)) x)")
         .fails("(?s)Incorrect syntax near the keyword 'SELECT'.*");
-    sql("SELECT map(SELECT x FROM (VALUES(1)) x, ^SELECT^ x FROM (VALUES(1)) x)")
-        .fails("(?s)Incorrect syntax near the keyword 'SELECT' at.*");
+    sql("SELECT map(SELECT x FROM (VALUES(1)) x^,^ SELECT x FROM (VALUES(1)) x)")
+        .fails("(?s)Encountered \", SELECT\" at.*");
   }
 
   @Test void testVisitSqlInsertWithSqlShuttle() {
@@ -7181,9 +7220,16 @@ private static Consumer<List<? extends Throwable>> checkWarnings(
         + "UNNEST(`DEPT`.`EMPLOYEES`, `DEPT`.`MANAGERS`)";
     sql(sql).ok(expected);
 
-    // LATERAL UNNEST is not valid
-    sql("select * from dept, lateral ^unnest^(dept.employees)")
-        .fails("(?s)Encountered \"unnest\" at .*");
+    // LATERAL UNNEST is the same as UNNEST
+    // (LATERAL is implicit for UNNEST, so the parser just ignores it)
+    sql("select * from dept, lateral unnest(dept.employees)")
+        .ok("SELECT *\n"
+            + "FROM `DEPT`,\n"
+            + "UNNEST(`DEPT`.`EMPLOYEES`)");
+    sql("select * from dept, unnest(dept.employees)")
+        .ok("SELECT *\n"
+            + "FROM `DEPT`,\n"
+            + "UNNEST(`DEPT`.`EMPLOYEES`)");
 
     // Does not generate extra parentheses around UNNEST because UNNEST is
     // a table expression.