Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24612 )
Change subject: IMPALA-15179: Support HBO for SelectNode cardinality ...................................................................... Patch Set 4: (2 comments) Went through PS4. The key shape (canonicalized conjuncts + child key, with operand qualification for multi-operand inputs) is right for what defines a SELECT node, and the e2e test exercises the partition-generalized child nicely. Two comments below. The limit one is the third instance of the same gap in this stack (24608 has it too, and IMPALA-15127's review fixed it in UnionNode/AggregationNode) - it may be worth hoisting the `limit:` component into shared code, e.g. the generateHboHashStrings() wrapper appending it for every node, in the spirit of Steve's polymorphism comment on 24608. http://gerrit.cloudera.org:8080/#/c/24612/4/fe/src/main/java/org/apache/impala/planner/SelectNode.java File fe/src/main/java/org/apache/impala/planner/SelectNode.java: http://gerrit.cloudera.org:8080/#/c/24612/4/fe/src/main/java/org/apache/impala/planner/SelectNode.java@158 PS4, Line 158: StringBuilder sb = new StringBuilder(statsType.name()).append(":SelectNode:"); limit_ is missing from the key, and a SelectNode can carry one: for a statement with LIMIT and no top-level ORDER BY the planner puts the limit on the root node (SingleNodePlanner.java:337), and a SELECT is the root in e.g. `select * from (select id, rank() over (order by id) rk from t) v where rk <= 5 limit 3`. The limited run then stores its capped row count under the same key the unlimited variant looks up, which adopts it with nothing to correct it (capCardinalityAtLimit() is a no-op without a limit). Same issue and fix as UnionNode/AggregationNode/SortNode's `limit:` component; needs the limited/unlimited regression pair too. http://gerrit.cloudera.org:8080/#/c/24612/4/fe/src/main/java/org/apache/impala/planner/SelectNode.java@170 PS4, Line 170: conjuncts_, CanonicalizationStrategy.EXPR_REWRITE, operandIdx); Why hardcode EXPR_REWRITE here instead of passing `strategy` through, as JoinNode does for its WHERE conjuncts_? As written, the IGNORE_PARTITION_CONSTANTS variant of this key generalizes the child scan's partition predicates but keeps the SELECT's own conjuncts literal - so if a partition predicate ever lands here (the TODO above asks exactly that), the generalized key still distinguishes the partition values and the strategy silently loses its purpose for this shape. Passing `strategy` through would keep the node consistent with JoinNode and costs nothing when no partition predicates are present. -- To view, visit http://gerrit.cloudera.org:8080/24612 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5804f9ebf9e06a4947ea3e11441c63e89c82e036 Gerrit-Change-Number: 24612 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 17 Aug 2026 08:57:20 +0000 Gerrit-HasComments: Yes
