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: (3 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: if (table == null) table = resolveTable(expr); > Nice catch! I think a simpler solution is just returning null in resolveTab Returning null covers both directions, including the case where the first resolved table has no clustering columns and suppressed generalization for another table's partition predicate. The new test does exercise the old path: partition keys are added first (HdfsTable.addColumnsFromFieldSchemas(msTbl.getPartitionKeys())), so alltypestiny has year/month at positions 0-1, and alltypesnopart.id sits at position 0. On PS14 referencesPartitionColumn() therefore returned true for a predicate whose only alltypestiny column is t1.id at position 2. 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() { > Added the cache for this map. Fine by me on the group collection. The cache has a second effect worth noting: PS14 rebuilt the map on every call, so the map used for the lookup in computeStats() and the one used for the store in toThrift() were separate objects built before and after invertJoins(). Caching pins both to the single-node phase. Same property as hboOrderedOperands_ - see the PlanNode thread. 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@1050 PS14, Line 1050: * depends only on table names, which are fixed after tree construction. Order-sensitive > > inversion doesn't change group membership invertJoin() is not restricted to directional joins. JoinOperator.invert() ends in "default: return this", so INNER, CROSS and FULL OUTER keep their operator but still go through Collections.swap(children_, 0, 1). isInvertible() only rules out straight joins, NULL AWARE LEFT ANTI, and - for non-local plans - left outer / left semi without eq conjuncts. Planner.invertJoins() reaches inner joins through isInvertedJoinCheaper(), which is the ordinary build/probe swap. So the join types that use this cached sorted list are exactly the ones that can be swapped. What I meant by group membership: the swap changes the traversal order in collectInnerCrossJoinGroup(), but not the set of operands, and sorting by scan table names absorbs the reordering - except on a tie, where thenComparingInt(originalIndex) falls back to the position in children_. "t a join t b" with different predicates on each side is such a tie. That matters because of the ordering: the cache is filled during single-node planning (JoinNode.computeStats() -> tryUpdateCardinalityFromHbo(), line 903), Planner.createPlanFragments() calls invertJoins() afterwards, and the store side runs later at toThrift() -> populateHboThriftFields(). Recomputing the list at that point would give a different operand order in the tie case, so stats would be stored under a key the lookup never produces. The cache is what keeps the two sides equal, which makes it an invariant rather than an optimization. That is all I am asking for here: a line saying it is deliberately not invalidated by invertJoin(), because the key generated during single-node planning has to stay equal to the key generated at toThrift() time. -- 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: Tue, 18 Aug 2026 10:08:22 +0000 Gerrit-HasComments: Yes
