Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24592 )
Change subject: IMPALA-15189: Support HBO for SortNode cardinality ...................................................................... Patch Set 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java File fe/src/main/java/org/apache/impala/planner/ExchangeNode.java: http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@441 PS11, Line 441: > I kinda feel like something is wrong here, overriding this. That's a good point. It's a bit messy due to maintaining the literal meaning of isCardinalityPreserving(). Instead, we should rename it to ignoredInHboCardKey(). Then ExchangeNode can always return true in it. PS12 removed these overrides. http://gerrit.cloudera.org:8080/#/c/24592/4/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/24592/4/fe/src/main/java/org/apache/impala/planner/JoinNode.java@1096 PS4, Line 1096: } > Should we put this and subtreeHasJoin in PlanNode? I think it's a little a It's removed in later patch sets. http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/SortNode.java@269 PS11, Line 269: PlanNode baseChild = getChild(0); > Nit: heh, you were the one who taught me this: Maybe change this to Done http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/SortNode.java@285 PS11, Line 285: } > Nit: this preconditions should be above the "if" clause at line 282 Done http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/SortNode.java@321 PS11, Line 321: if (mergeParent_ instanceof ExchangeNode) { > If the value changes only because its mutated, can we just keep the origina We keep the original offset (originalOffset_) and we can use it here. But keeping and using the original limit will be tricky since getSortLimit() could return limitWithTies_ instead of limit_. We will still need a check like "mergeParent_ instanceof ExchangeNode" so not reducing the lines of changes. http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/SortNode.java@474 PS11, Line 474: } > Nit and suggestion: Done -- To view, visit http://gerrit.cloudera.org:8080/24592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib829887a91593bee124d56e661e714575fe3be97 Gerrit-Change-Number: 24592 Gerrit-PatchSet: 12 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-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 29 Sep 2026 13:18:13 +0000 Gerrit-HasComments: Yes
