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 11: (4 comments) Another pass, but I spent a lot of time on the last comment here. If you don't agree with my comment, I'll re-review what is there, since I'm suggesting the removal of a couple of different methods which I'm not looking at if you decide to move forward with my comment. http://gerrit.cloudera.org:8080/#/c/24426/20/fe/src/main/java/org/apache/impala/planner/ExprCanonicalizer.java File fe/src/main/java/org/apache/impala/planner/ExprCanonicalizer.java: http://gerrit.cloudera.org:8080/#/c/24426/20/fe/src/main/java/org/apache/impala/planner/ExprCanonicalizer.java@145 PS20, Line 145: /** I think "expr.collect(SlotRef.class, slotRefs)" can be used instead of getNodesPreOrder? Not much of a change, but slightly less code. http://gerrit.cloudera.org:8080/#/c/24426/20/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/20/fe/src/main/java/org/apache/impala/planner/JoinNode.java@942 PS20, Line 942: * Builds a map from each operand subtree's TupleIds to that operand's canonical index, I just learned something new today! First time I've seen this in the wild. http://gerrit.cloudera.org:8080/#/c/24426/20/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/20/fe/src/main/java/org/apache/impala/planner/PlanNode.java@219 PS20, Line 219: /** Nit: carriage return http://gerrit.cloudera.org:8080/#/c/24426/20/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1103 PS20, Line 1103: LOG.debug("No HBO stats for {}. Keys: {}", getDisplayLabel(), hashKeys); Suggestion on this: The buildHboOperandQualifierMap seems like it could be a little simpler (perhaps just imo?). I don't particularly like the "instanceof JoinNode" check here. It seems like this whole thing could be done more simply through just recursion, maybe?. Also, the "Computed_" variable seems extra: i could be missing some of the nuances, but it seems an empty map is the same as a null, so perhaps the extra variable can be avoided. I'm also not a big fan of mutating variables that are passed in, and I think that also can be avoided. I think that confuses the implementation for me as I read it. So I may be missing some nuances here and got this code wrong, and maybe I'm not taking into account future commits, But maybe we can rewrite like this? ... - call hboOperandQualifierMap_ = buildHboOperandQualifierMap("") at init time when Hbo is set (maybe even for all nodes? Not sure it matters). Does invertJoins change this? If so, I think that can be worked in as well in the "join" code, but I'll ignore that for now. - Keep the return type for the buildHboOperandQualifierMap as Map<TupleId, String> - one operand: "String prefix" - At PlanNode level: if prefix is not blank, create the tuple map with the given prefix and return the map. If prefix is blank, return empty hash map. - At any node where "skipOperandTransparentNodes" returns true: Preconditions check it only has one child. Return child.buildHboOperandQualifierMap(prefix) - Agg node: if prefix is not empty, return super.buildHboOperandQualifierMap(prefix). If its child multiAggInfo == its multiaggInfo, return child.buildHboOperandQualifierMap(prefix). else return empty map - Join node: create empty hash map 'myMap". For each operand in getHboOperands(), myMap.addAll(operand.buildHboOperandQualifierMap(extendPath(prefix, i)); Obviously haven't tested this, and I hope I didn't miss anything or screw up my understanding of the logic. But this would remove the need for the "skip", the "append", the "getHboBaseChild", place the join logic where it belongs, and imo, both keep it fairly simple and more easy to understand. Thoughts? -- 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: 11 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: Thu, 20 Aug 2026 20:07:04 +0000 Gerrit-HasComments: Yes
