Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24426 )
Change subject: IMPALA-14601: Support HBO for JoinNode cardinality ...................................................................... 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. 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 (and replace the "isCardinalityPreserving") is to call this getCardinalityPreservingNode() in planNode, return "this" by default, and have an override on the "skip" ones which returns its child. http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/JoinNode.java@926 PS22, Line 926: static PlanNode skipOperandTransparentNodes(PlanNode node) { I think this is dead code now? http://gerrit.cloudera.org:8080/#/c/24426/22/fe/src/main/java/org/apache/impala/planner/JoinNode.java@939 PS22, Line 939: private static void collectInnerCrossJoinGroup(JoinNode node, List<JoinNode> groupNodes, Gonna leave a totally optional comment because this is personal preference on two levels: Feel free to ignore both. 1) Is groupNodes really needed? If I'm reading this correctly, this will always be a part of operands. While I suppose it makes the for loop in collectFlattenGroupOperands a little smaller, it also comes with a cost of adding the node to two different arrays. Not sure there is a net savings. I guess it also skips over checking if the join is an inner or cross join too? Not too much of a savings there either, I don't think. But this suggestion is more for aesthetic reasons, so I'm ok if you leave it as/is. 2) If we get rid of groupNodes, then my next suggestion would be to have this return a List<PlanNode> rather than make this a "collect" method. Again for "immutable" reasons. It would necessitate returning a new array so perhaps this way is more efficient. I'll leave it up to you, but that's how I would have coded it. 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 variable? Can we just call the "buildHbo..." within the "generateHbo" since it's only called once (for each needed node)? Should we cache the "generateHboKeyString" (the map of types of strings) instead? I suppose special care would have to be taken if the generate function returned null for a specific type map...instead of putting in null, some placeholder string perhaps. 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! We could still get rid of "isOperandTransparent()" also by moving this "if" code into the "transparent" nodes. Actually, we probably could get rid of the "Preconditions" statement, since it is implied in those nodes (an ExchangeNode will never have more than 1 child, I bet that's a precondition elsewhere). I like that better, but if you like it here, I wouldn't put up a fuss. -- 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: 22 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: Mon, 24 Aug 2026 22:20:41 +0000 Gerrit-HasComments: Yes
