Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24592 )
Change subject: IMPALA-15189: Support HBO for SortNode cardinality ...................................................................... Patch Set 11: (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: public String generateHboKeyString(THboStatsType statsType, I kinda feel like something is wrong here, overriding this. This is just like the parent version without the "isCardinalityPreserving()". So I get why this is special, but I'm thinking there should be another method used in the parent, like "shouldGenerateHbo()", so that this isn't overridden (also applies to appendScanInputStats). Then, the parent "shouldGenerateHbo()" returns "isCardinalityPreserving()" by default, and the "shouldGenerateHbo()" in this class is overridden to be "true", since that's what makes the ExchangeNode special. 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: return generateFlattenJoinGroupKeyString(statsType, strategy); Should we put this and subtreeHasJoin in PlanNode? I think it's a little awkward to have AggregationNode and SortNode call a method in JoinNode directly. 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: while ((baseChild instanceof SortNode Nit: heh, you were the one who taught me this: Maybe change this to while ((baseChild instanceof SortNode sortChild && sortChild.info_ == info_) || baseChild instanceof ExchangeNode) { http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/SortNode.java@285 PS11, Line 285: Preconditions.checkState(children_.size() == 1); Nit: this preconditions should be above the "if" clause at line 282 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 original value here? Or is that value lost because the PlanNode is reconstructed? http://gerrit.cloudera.org:8080/#/c/24592/11/fe/src/main/java/org/apache/impala/planner/SortNode.java@474 PS11, Line 474: } else { Nit and suggestion: I see this clear is at the top of "tryUpdate..." But would it be better to put this clear code in PlanNode.computeStats()? Or would this cause issues? -- 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: 11 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: Wed, 23 Sep 2026 18:02:49 +0000 Gerrit-HasComments: Yes
