Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24608 )
Change subject: IMPALA-15175: Support HBO for AnalyticEvalNode cardinality ...................................................................... Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/24608/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/24608/4/fe/src/main/java/org/apache/impala/planner/JoinNode.java@1165 PS4, Line 1165: if (node instanceof AnalyticEvalNode) { I have a general comment here. I know this should be reviewed after several of the other HBO PlanNode commits, but I think this is a good place to talk about what I'd like to mention... This is the third "if" condition that uses "instanceof" for a PlanNode. Whenever I see an "instanceof", I usually like to ask the question: Is it possible to place this code in the class itself and use polymorphism to find the right code? I think the fact that there are 3 "if"s here means that this could be designed differently? In order to put this code in AnalyticEvalNode, the general methods here have to change, and the change would have to happen across multiple commits. To further clean the code, it would expand to changing "buildHboInputQualifierMap" into a non-static method, and then "subtreeHasJoin" probably can change (and perhaps others?) which seems to fit under this same umbrella. I don't feel too comfortable making code review suggestions like this since it may involve a bit of work. And I'm not sure if this falls under the category of "good maintenance" code that should be changed or "I don't agree with this style" code that can be ignored. If you'd like to wait for another reviewer before changing, I understand. -- To view, visit http://gerrit.cloudera.org:8080/24608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d87ab472db9273622f48fa62bb1d8c12e04e86b Gerrit-Change-Number: 24608 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Thu, 30 Jul 2026 14:56:31 +0000 Gerrit-HasComments: Yes
