Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24257 )
Change subject: IMPALA-14940: Calcite planner: handle broadcast and shuffle hints. ...................................................................... Patch Set 7: (3 comments) The join-without-ON fix throws at runtime - details inline, along with a narrower form that also keeps CROSS JOIN parsing. On hint scoping and validation, I went through the test corpus and agree with deferring both. http://gerrit.cloudera.org:8080/#/c/24257/7/java/calcite-planner/src/main/codegen/templates/Parser.jj File java/calcite-planner/src/main/codegen/templates/Parser.jj: http://gerrit.cloudera.org:8080/#/c/24257/7/java/calcite-planner/src/main/codegen/templates/Parser.jj@1973 PS7, Line 1973: <HINT_BEG> AddHint(hints) ( <COMMA> AddHint(hints) )* <COMMENT_END> Agreed, let's defer it - and the pooling costs less than I assumed. In the e2e joins.test the hints are uniform within a query block (all [shuffle] at 434-441, all [broadcast] at 454-461), and those cases assert results rather than plan shape. The only mixed-hint query I found is PlannerTest/with-clause.test:442, which runs on the original planner. So nothing in the Calcite path depends on per-join scope right now, and a comment plus the Jira is enough. One idea for that Jira: put the hints on the right-hand table ref instead of on SqlSelect. Calcite supports table-ref hints natively and propagates them down to the scan, and it matches where the original planner keeps them - TableRef.analyzeJoinHints() stores the hint on the joined TableRef. It does not cover a subquery on the right side, but it may beat encoding SqlNode positions into SqlHint options. http://gerrit.cloudera.org:8080/#/c/24257/7/java/calcite-planner/src/main/codegen/templates/Parser.jj@2094 PS7, Line 2094: String joinTypeString = (String) SqlLiteral.value(joinType); Three problems here, and the code-review-checks build stops before the precommit tests, so none of them would show up there. `SqlLiteral.value()` returns the symbol's enum, not a String. `joinType` is the literal built by `JoinType()` via `joinType.symbol(getPos())`, so its typeName is SYMBOL, and for symbols `SqlLiteral.value()` returns `(Enum<?>) literal.value` (SqlLiteral.java:468-469 in calcite 1.42). The `(String)` cast compiles because String is Comparable, but throws at runtime. Ran it against calcite-core 1.42.0 / avatica 1.23.0, the versions from java/calcite-planner/pom.xml: INNER: typeName=SYMBOL valueClass=org.apache.calcite.sql.JoinType value=INNER (String) cast THREW: class org.apache.calcite.sql.JoinType cannot be cast to class java.lang.String getValueAs(JoinType.class) = INNER Same for LEFT and CROSS, so every join without ON/USING throws here, including the query in the commit message. Second, once the cast is fixed the check rejects CROSS JOIN. `<CROSS> <JOIN>` is matched by `JoinType()`, and with no ON/USING it reaches this same branch, so `select ... from a cross join b` stops parsing. That is valid Impala syntax (sql-parser.cup:3506), and joins.test:151 covers it - start-impala-cluster.py passes -use_calcite_planner=true cluster-wide when USE_CALCITE_PLANNER=true, so that test runs through this path. Third, `natural` is dropped and replaced by createBoolean(false). NATURAL JOIN has joinType INNER, so it passes the check and silently becomes a comma join. The original parser has no grammar rule for NATURAL at all, so a query that errors today would come back as a cross product. Narrowing the rewrite and leaving the rest to the validator covers all three: if (joinType.getValueAs(JoinType.class) == JoinType.INNER && !natural.booleanValue()) { // Impala extension: "FROM t t1 JOIN t t2" with no ON or USING clause. return new SqlJoin(joinType.getParserPosition(), e, SqlLiteral.createBoolean(false, joinType.getParserPosition()), JoinType.COMMA.symbol(joinType.getParserPosition()), e2, JoinConditionType.NONE.symbol(joinType.getParserPosition()), null); } return new SqlJoin(joinType.getParserPosition(), e, natural, joinType, e2, JoinConditionType.NONE.symbol(joinType.getParserPosition()), null); CROSS and COMMA pass validateJoin() unchanged, and LEFT/RIGHT/FULL without a condition hit Calcite's joinRequiresCondition() (SqlValidatorImpl.java:4102-4108), which carries the parser position - a raw ParseException does not. Worth adding a case for `from t t1 join t t2` while you are here. QueryTest/calcite has no join without ON, and the "cross join test" there is comma syntax, so neither of the first two would be caught by the calcite suite. http://gerrit.cloudera.org:8080/#/c/24257/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java: http://gerrit.cloudera.org:8080/#/c/24257/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@113 PS7, Line 113: JoinNode.DistributionMode distMode = containsHint(SHUFFLE) Agreed, same Jira. I did look for a subset of the validation that would survive without per-join scope, and there isn't one - once the hints are pooled, [broadcast, shuffle] on a single join is indistinguishable from one hint on each of two joins. -- To view, visit http://gerrit.cloudera.org:8080/24257 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie5a932cc09d3d370aa627228d0e226a92fa03168 Gerrit-Change-Number: 24257 Gerrit-PatchSet: 7 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Fri, 07 Aug 2026 07:55:10 +0000 Gerrit-HasComments: Yes
