Joe McDonnell 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: (2 comments) I wanted to see what was possible here, so I hacked on an alternate version of this. It reuses most of the concepts here, except that it subclasses AnalysisContext/AnalysisResult directly rather than having an AnalysisDriver. It also has a single factory rather than using chaining. I didn't spend time on the frontend tests except to get them to compile, but it shouldn't be hard to use the factory in the frontend tests. I moved the fallback to what I think is the right place (Frontend::createExecRequest()). Take a look here: http://gerrit.cloudera.org:8080/22475 Based on my exploration, I don't think the AnalysisDriver is needed. I'm a bit ambivalent about chaining versus factory, but I do think it is awkward to have a ParsedStatement know how to instantiate an AnalysisContext and an AnalysisResult know how to instantiate a SingleNodePlanner. http://gerrit.cloudera.org:8080/#/c/22306/8/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/8/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@490 PS8, Line 490: // TODO: hack! Having the singleNodePlanner_ set within the analysisResult : // is not a good thing to do. It really doesn't belong here. : // However, the TResultSetMetadata for CalcitePlanner is only created after the : // single node planner is run, but the regular Impala planner fetches the info : // off of AnalysisResult in a place where the SingleNodePlanner is not available. : // Some major refactoring should be done to make this cleaner in order for both : // compilation paths to work, but in the meantime, we set the SingleNodePlanner : // within the AnalysisResult so that the TResultSetMetadata can be fetched from : // one place. Here are two options: 1. getPlannedExecRequest()/createExecRequest() could return a structure that contains a TQueryExecRequest and other things like the TResultSetMetadata (only set for queries). My guess is that this could be a useful concept. There are other things that use Analyzer/AnalysisResult as plumbing out of Planner, and this could avoid that. 2. Frontend's PlanCtx (which probably should have a different name, given how close it is to PlannerContext) is being passed through these functions and can be used to pass information out. Basically, if we have a TResultSetMetadata field that is set in createExecRequest() only for queries, we can read it back in that location in doCreateExecRequest(). The second is hackier, but easier. http://gerrit.cloudera.org:8080/#/c/22306/8/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@585 PS8, Line 585: public static class AnalysisDriverImpl implements AnalysisDriver { I don't think the AnalysisDriver concept is necessary. It would be better to reuse the AnalysisContext / AnalysisResult concepts. I'm posting some code to show one way to do that. Basically, CalciteAnalysisContext subclasses AnalysisContext and CalciteAnalysisResult subclasses AnalysisResult. -- 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 <[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-Comment-Date: Tue, 11 Feb 2025 23:54:23 +0000 Gerrit-HasComments: Yes
