[Impala-ASF-CR] IMPALA-15179: Support HBO for SelectNode cardinality

["Aleksandr Efimov (Code Review)" <ge...@cloudera.org>]

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 <hu...@gmail.com>
Gerrit-Reviewer: Aleksandr Efimov <dz...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Aug 2026 08:57:20 +0000
Gerrit-HasComments: Yes