Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend ...................................................................... Patch Set 8: I looked at the alternate implementation and I disagree with the design decision of subclassing AnalysisContext and AnalysisResult. I feel that an alternative planning mechanism, should have an API that is as simple as possible. The top level of simplicity would be the number of classes defined within the API. To me, it makes more sense to have only one API class/interface for parsing (we do), one API class/interface for the Planner (we do), but also only one API class/interface for Analysis. The alternative proposal forces an API developer to derive from the 2 classes AnalysisContext and AnalysisResult. The second issue I have with the alternative proposal (kinda similar to the first objection I have) is that I believe the API should be a pure interface, and then we should implement the interface with the legacy planner and the Calcite planner. An interface keeps both implementation with an equal footing and pushes shared code into a location which can be used by both implementations. The alternative proposal treats AnalysisContext and AnalysisPlanner as base classes. Some of the code is then used in the base class and some of the methods are overridden. I find this confusing when doing code inspection on a new piece of code. I have to check to look at the default legacy implementation in the base class but then look in the derived Calcite class to see if that specific method was overridden. The one advantage I see for using the alternative approach is that anyone familiar with the current code might feel more comfortable with keeping the familiar classes? And it's less disruptive for someone who knows the legacy code? I don't feel this is enough of a reason to go with that approach since the whole Analysis module should probably be rewritten (e.g. AnalysisContext is not really a context as defined by most code) and it's already confusing right now. -- To view, visit http://gerrit.cloudera.org:8080/22306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d Gerrit-Change-Number: 22306 Gerrit-PatchSet: 8 Gerrit-Owner: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Feb 2025 19:08:24 +0000 Gerrit-HasComments: No
