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 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@575 PS11, Line 575: public static class AnalysisDriverImpl implements AnalysisDriver { > The old code had a AnalysisContext that produces an AnalysisResult. It wasn I'm not sure I quite understand what the division of responsibilities is. I guess I can try... AnalysisContext isn't really a context. It seems kinda like the Driver? AnalysisResult also isn't really a Result class since it contains the Analyzer? Which I suppose contains results, but then it contains results within the Analyzer and outside the Analyzer? And then we make calls off of the analysisResult stmt like analysisResult_.stmt_.analyze(analysisResult_.analyzer_)? Which a) accesses a member variable of AnalysisResult, b) calls a method off of it(???), and c) passes in a third object which is a member variable that is mutated(???) Mutating a variable like this while containing other mutations is a big no-no for me and I'm having a hard time wrapping my head around this without a big rewrite. So the Context class isn't really a Context class and the Result class isn't really a Result class. Is it a wrapper class and an implementor/result-ish class? So I had 2 goals in mind with any design, and I have to stay true to these ideals for anything I write. I thought I did that here, but I can try again to see if I can come up with something we both find palatable. Those 2 goals are: 1) Keep the changes somewhat minimal. Any code review is going to be difficult. The more stuff I move around, the more prone to error this is and the more difficult it will be to review. I think I accomplished that here...while the review has 700+ lines, half of the changes within AnalysisContext which are just adding "ctx_" and adding some member variables. I kept logic flow changes to a minimum, I think. 2) The design has to have pure interfaces. Shared logic should be in a shared class, but the shared class should not contain any method that is implementation specific. This doesn't fly for me because it's the type of class that is very hard to debug by code inspection since one has to ignore the base class method and specifically remember (or read a comment) that it is overridden in a derived class. I think I accomplished my goals here. I don't really think I made anything worse by creating a wrapper around the code that exists. IMO, we should create a Jira to do a general rewrite rather than try to figure out a compromise API using 2 classes which is why I chose this implementation. And I think that general rewrite is too big of a change for what is trying to be accomplished in this code review. And 2) is kinda my problem here. Some of AnalysisContext is implementation specific. Some of it is not. I can move the implementation specific code into a separate interface/derived class? But then the code review becomes a little tougher. Or I can keep it all in one file and immediately break it up into a different file in a second code review? I can see what that looks like, I suppose, but while you don't place any importance on 2 separate analysis API classes, I kinda find it awkward. Even worse for me for case 2) is that I don't think I can create a CalciteAnalysisResult class (as proposed in another PR) and feel good about it. Because of the nature of accessing member variables directly, it might be impossible to create an interface...it might have to be a derived class. And I don't think I feel comfortable writing that. But I can try and see what it looks like. Anyway, I'll look at this a little more but I wanted to share my initial thoughts before I dig in and try to redesign something we both can agree upon. -- 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: 11 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 04 Mar 2025 06:43:23 +0000 Gerrit-HasComments: Yes
