Steve Carlin 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 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/24257/6/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/6/java/calcite-planner/src/main/codegen/templates/Parser.jj@1973 PS6, Line 1973: <HINT_BEG> AddHint(hints) ( <COMMA> AddHint(hints) )* <COMMENT_END> > These join hints all end up in the same `SqlSelect.hints` list. Calcite pro Yeah, I looked into this before and discussed this with someone internally months ago... The problem is that the hints are attached to the SqlSelect which can have the chain of joins. While we differentiate the hints per join, there's no straightforward way to do that within Calcite that I saw. I could easily have missed something, of course. So I suppose we can attach whatever information we want within the SqlHint, including information about the SqlNode. But Calcite is still gonna handle this generically up until the RelNodeConverter time. At that point, we could still do something, but it starts to get a little clunky. I'm definitely open to ideas. But for the first pass, we decided internally that we could live with this limitation. One of the main reasons to get this in as soon as possible was to allow e2e tests to pass and this does the job. I'm not sure how many people are using these hints outside of the test framework. There are some other query options that help out with broadcast versus partitioned joins as well. So I'll mention this in the comment, file a Jira for it, and fix this at a later time, if that's ok. But if you do have an easy (or fairly easy) idea on how to implement this, I'm all ears on that. http://gerrit.cloudera.org:8080/#/c/24257/6/java/calcite-planner/src/main/codegen/templates/Parser.jj@2094 PS6, Line 2094: return new SqlJoin(joinType.getParserPosition(), > Could we limit this rewrite to a plain/inner join? As written, `LEFT`, `RIG Done http://gerrit.cloudera.org:8080/#/c/24257/6/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/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@113 PS6, Line 113: JoinNode.DistributionMode distMode = containsHint(SHUFFLE) > Could we keep the same hint validation as `TableRef.analyzeJoinHints()`? `[ This sorta has the same problem as what I put in the first comment. If I can't limit the scope of the hint, then this logic gets a little messed up. -- 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: 6 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 00:10:03 +0000 Gerrit-HasComments: Yes
