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 15: (4 comments) Thanks for the detailed review! Addressed the comments. http://gerrit.cloudera.org:8080/#/c/24426/14/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/14/fe/src/main/java/org/apache/impala/planner/ExprCanonicalizer.java@254 PS14, Line 254: && table.referencesPartitionColumn(expr)) { > resolveTable() picks the table of the first bound SlotRef, but referencesPa Nice catch! I think a simpler solution is just returning null in resolveTable() if 'expr' references multiple tables. Such predicates are not partition equality predicates that we want to canonicalize. Added a FE unit test for this. http://gerrit.cloudera.org:8080/#/c/24426/14/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/14/fe/src/main/java/org/apache/impala/planner/JoinNode.java@1039 PS14, Line 1039: private Map<TupleId, String> hboOperandQualifierMap_; > Every key generation rebuilds this map, and generateHboHashStrings() does i Added the cache for this map. For reusing the group collection in collectInnerCrossJoinGroup(), I think the improvement is marginal since traversal is cheap and bounded by the number of contiguous inner joins. The expensive part is the per-strategy predicate canonicalization and string building which we don't cache. But we can revisit this in the future. http://gerrit.cloudera.org:8080/#/c/24426/14/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/14/fe/src/main/java/org/apache/impala/planner/PlanNode.java@71 PS14, Line 71: import org.apache.impala.util.BitUtil; > Duplicate import: TScanInputStats is already imported on line 68. Done http://gerrit.cloudera.org:8080/#/c/24426/14/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1050 PS14, Line 1050: * nodes may override this: JoinNode suppresses the sort for directional joins. > inversion doesn't change group membership I'm not sure if I fully understand this. JoinNode.invertJoin() is only used on directional joins, not INNER/CROSS joins that have flatten groups and need sorting here. For directional joins, there are no groups, just the left and right operands. JoinNode.getHboOrderedOperands() has a simple handling on directional joins: @Override protected List<PlanNode> getHboOrderedOperands() { if (joinOp_.isInnerJoin() || joinOp_.isCrossJoin() || joinOp_.isFullOuterJoin()) { return super.getHboOrderedOperands(); } if (joinOp_.isRightHandedJoin()) { return Arrays.asList(getChild(1), getChild(0)); } return children_; } -- 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: 15 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-Comment-Date: Tue, 18 Aug 2026 09:23:48 +0000 Gerrit-HasComments: Yes
