Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24426 )
Change subject: IMPALA-14601: Support HBO for JoinNode cardinality ...................................................................... Patch Set 14: (4 comments) Went through PS14. The right-handed inversion is consistent across the key string, invertJoin(), and appendScanInputStats(), and the updated hbo-multiple-scans goldens match the actual row counts. A few comments below. 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: if (table == null) table = resolveTable(expr); resolveTable() picks the table of the first bound SlotRef, but referencesPartitionColumn() then checks every SlotRef's column position against that single table's clustering-column count, so multi-table conjuncts can mix tables. Example: a join-level WHERE conjunct `t1.int_col IN (t2.part_col, 5)` (not pushable to either scan). resolveTable() returns t1; the check sees t2's partition column at position 0 and enables constant removal, so under IGNORE_PARTITION_CONSTANTS the 5 becomes <CONST> in a predicate on a non-partition column of t1, and runs that differ only in that constant share stats. In the opposite direction (first table has no clustering columns) the early return suppresses generalization for the other table's partition predicate. Rare shapes, but reachable through join WHERE conjuncts. Could the table be resolved per SlotRef? The tuple-to-operand mapping already exists in buildHboOperandQualifierMap(), so the pairing is available. 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: Map<TupleId, String> buildHboOperandQualifierMap() { Every key generation rebuilds this map, and generateHboHashStrings() does it once per strategy; the AggregationNode above the join builds the same map again, collectFlattenGroupPredicates() re-collects the group that getFlattenOperands() already walked, and qualifyForHbo() deep-clones each conjunct. On wide inner-join groups (TPC-DS shapes) this adds up in the planning path - computeStats() and toThrift() both go through it. The map depends only on the operand structure; could it be cached next to hboOrderedOperands_, and the group collection reused? 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.thrift.TScanInputStats; Duplicate import: TScanInputStats is already imported on line 68. http://gerrit.cloudera.org:8080/#/c/24426/14/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1050 PS14, Line 1050: * depends only on table names, which are fixed after tree construction. Order-sensitive This justification predates join-group flattening: for a JoinNode the operand list now depends on the group's shape, not just table names, and the cache is typically populated during computeStats(), i.e. before DistributedPlanner may call invertJoin(). As far as I can tell it stays self-consistent - the cached list feeds both the key and appendScanInputStats(), inversion doesn't change group membership, and directional joins bypass the cache - but that invariant is worth stating here, since a call site that regenerates the key after a structural change would silently disagree with the cache. -- 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: 14 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: Mon, 17 Aug 2026 07:35:41 +0000 Gerrit-HasComments: Yes
