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 8: (4 comments) Patch Set 8: (4 comments) The parser fix checks out. One question on the hint strategy registration, the rest are nits on the new test. http://gerrit.cloudera.org:8080/#/c/24257/8/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/8/java/calcite-planner/src/main/codegen/templates/Parser.jj@1973 PS8, Line 1973: <HINT_BEG> AddHint(hints) ( <COMMA> AddHint(hints) )* <COMMENT_END> The hint does not have to be lost - Calcite has a second place to hang hints, on the table ref rather than on SqlSelect. TableRef1 and friends already do `( tableRef = TableHints(tableName) | { tableRef = tableName; } )` (Parser.jj:1701, 1747, 1778, 1816, 2181), and TableHints() wraps the identifier in a SqlTableRef carrying its own SqlNodeList of hints. SqlToRelConverter handles SqlKind.TABLE_REF and passes that list through convertIdentifier(bb, id, extendedColumns, tableHints) into toRel(RelOptTable, List<RelHint>), so it lands on the scan. The catch is the syntax position: Calcite's own table hints come after the table name, while Impala writes the hint before it. So it is not free syntax - it would mean routing the hint list that JoinType() already parses onto e2 inside JoinTable() instead of into the select-level list, and registering with HintPredicates.TABLE_SCAN. And it does not cover a subquery on the right-hand side. Your idea of extending SqlHint with join information would cover that case too, so it may well be the better one - I have not tried either. Either way it is IMPALA-15251 material, no need to settle it here. http://gerrit.cloudera.org:8080/#/c/24257/8/java/calcite-planner/src/main/codegen/templates/Parser.jj@2094 PS8, Line 2094: if (joinType.getValueAs(JoinType.class) == JoinType.INNER Checked PS8: natural comes from Natural(), which always returns a boolean literal, so there is no null path, and CROSS/COMMA now fall through to the generic SqlJoin. Looks right. One FYI, no action needed: NATURAL JOIN now reaches Calcite as a real natural join instead of the silent cross product it was before. Since the original parser has no NATURAL rule at all, that is a query which errors today and returns rows on the Calcite path. Strictly better than what it did before, just worth knowing it is there. http://gerrit.cloudera.org:8080/#/c/24257/8/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaCoreRules.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaCoreRules.java: http://gerrit.cloudera.org:8080/#/c/24257/8/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaCoreRules.java@231 PS8, Line 231: .hintStrategy("shuffle", HintStrategy.builder(HintPredicates.JOIN) A question, and I may be missing something here: shuffle and broadcast are registered with excludedRules(JOIN_TO_MULTI_JOIN), the same as straight_join. As far as I can tell that keeps a hinted join out of the MultiJoin, so LoptOptimizeJoinRule never gets to reorder it - the exclusion is enforced in AbstractRelOptPlanner.fireRule via RelOptRuleCall.isRuleExcluded(). In the original planner join order depends only on isStraightJoin() (SingleNodePlanner.java:733, 950); distrMode_ is orthogonal, so [broadcast] there fixes the distribution and leaves the ordering to the cost model. And since the hints are pooled on SqlSelect, one [broadcast] anywhere in a query block would freeze the order for every join in it. Is that deliberate - the hints would not survive the MultiJoin round trip otherwise? If so, a line in the commit message or on IMPALA-15251 would help, since it is a bit broader than the scoping problem itself. I have not run a plan to confirm the ordering actually changes, so treat this as a question rather than a finding. http://gerrit.cloudera.org:8080/#/c/24257/8/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/24257/8/testdata/workloads/functional-query/queries/QueryTest/calcite.test@1381 PS8, Line 1381: #Test cross-join syntax Nits, take or leave: There is a leftover `---- TYPES / STRING` above RESULTS, copied from the unhex test. It is harmless - parse_test_file_text keeps the last section with a given name (test_file_parser.py:306), so INT, INT is what gets used - but it reads as if the test expected a string. The comment says cross-join syntax while the query is JOIN with no ON, which is the Impala extension. The `cross join` keyword hits the same branch and was the other case that broke in PS7, and `left join` with no ON now lands on joinRequiresCondition() instead of a ParseException. Two more queries would pin both down, but neither is essential: the keyword is covered by joins.test:151 on a full run with USE_CALCITE_PLANNER=true, and nobody writes a LEFT JOIN without ON on purpose. -- 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: 8 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 20:21:56 +0000 Gerrit-HasComments: Yes
