Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24426 )
Change subject: IMPALA-14601: Support HBO for JoinNode cardinality ...................................................................... Patch Set 23: (6 comments) > Patch Set 22: > > (5 comments) > > Sorry about this second code review. The last one which changed the code > involved a lot of time for me to try to figure out what was going on and how > it could be improved. But thank you for making those changes! > > This round isn't too bad and mostly optional stuff. > > One favor: The Union patch wound up causing a failure for the > IS_CALCITE_PLANNER=true option which I need to investigate. If you have a > chance, can you run your tests with USE_CALCITE_PLANNER=true? If anything > fails, no need to fix, just disable the test, file a Jira, and you can assign > it to me. No worries if you don't do this...just saves me some time up front. Thanks for the comments and they do make the code cleaner! Updated the tests to pass with calcite-planner. http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/JoinNode.java File fe/src/main/java/org/apache/impala/planner/JoinNode.java: http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/JoinNode.java@914 PS22, Line 914: private static PlanNode skipCardinalityPreservingNodes(PlanNode node) { > Optional: Another way of doing this would be to eliminate this function (an There are some customized usages on isCardinalityPreserving(), e.g. in AggregationNode.getHboBaseChild(). How about doing this refactor after we support all node types, so all the usages are clear? http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/JoinNode.java@926 PS22, Line 926: */ > I think this is dead code now? Oops, removed this. http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/JoinNode.java@939 PS22, Line 939: } > Gonna leave a totally optional comment because this is personal preference Operands are the "real" children of the JoinNodes. Take the following plan as an example: JoinNode1 |-- JoinNode2 | |-- ScanNode C | ScanNode B ScanNode A groupNodes is [JoinNode1, JoinNode2] and operands is [ScanNode A, B, C]. So I think we need two lists. http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/PlanNode.java@220 PS22, Line 220: private Map<TupleId, String> hboOperandQualifierMap_; > Actually now that I see the new code...is there a reason to have this varia It will need a broader (and probably invasive and risky) change to cache the HBO key strings since they are generated in computeStats() which could be invoked in several phases, e.g. in SingleNodePlanner or in DistributedPlanner after some modification on the nodes. I plan to work on this after HBO supports all node types and maybe all stats types. There are TODOs about this in generateHboKeyString but I should file a JIRA for this. Before that I think we still need the cached hboOperandQualifierMap_. http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1098 PS22, Line 1098: if (isOperandTransparent()) { > I definitely like this better, thanks! Currently only ExchangeNode and TupleCacheNode return true in isOperandTransparent(). Indeed, SortNode and AnalyticEvalNode should also return true (and there might be more node types). I will add that for follow-up patches which adds HBO support for them. I think moving this "if" code to those nodes is a duplication. Or we need a class for OperandTransparentNode and let them inherrit it and put this "if" code there. Maybe the current code is simpler. http://gerrit.cloudera.org:8080/#/c/24426/22/tests/query_test/test_hbo.py File tests/query_test/test_hbo.py: http://gerrit.cloudera.org:8080/#/c/24426/22/tests/query_test/test_hbo.py@322 PS22, Line 322: self._run_hbo_explains('QueryTest/hbo-outer-join') Removed conditions on alltypestiny otherwise it's equivalent to INNER JOIN and Calcite will convert the LEFT OUTER JOIN to INNER JOIN. -- To view, visit http://gerrit.cloudera.org:8080/24426 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70b655ae7027d0d9eb8e9fae9ba2e1b7ad9876b4 Gerrit-Change-Number: 24426 Gerrit-PatchSet: 23 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 26 Aug 2026 14:32:21 +0000 Gerrit-HasComments: Yes
