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 11: (14 comments) 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@98 PS11, Line 98: // Hack: stmt_ and analyzer_ are not final because of the "rewrite" Drop "Hack:" I would use "because rewrites overwrite these fields" 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't perfect, but it had a division of responsibilities. The AnalysisDriver borrows a mixture responsibilities from the other two, and the resulting design doesn't really have a clear set of responsibilities for any of the classes. AnalysisDriver is a field on AnalysisResult to convey extra information out of analysis, but it also creates the Analyzer and implements analyze(), which fills in its parent class. It functions, but it is a weird design. I want something where the classes have a clearer set of responsibilities. I don't place any importance on overriding a single class rather than two classes. http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@586 PS11, Line 586: ctx_ = analysisContext; Style-wise, we only use the analysisResult_ field on ctx_ and we use it in many locations. I think we should have an analysisResult_ field on this so that we don't need to do ctx_.analysisResult_ every time. http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java File fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java: http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java@30 PS11, Line 30: raferenced Nit: referenced http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java File fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java: http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@45 PS11, Line 45: // the wrapped sql statement object. Note, this is not final because Impala : // allows the statement to be rewritten at analysis time (which is kinda : // hacky) : private StatementBase stmt_ ; If I'm understanding this right, we don't use setTopLevelNode() anywhere, so we can remove that and make this final. http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@78 PS11, Line 78: // set the wrapped statement on rewrite. : public void setTopLevelNode(StatementBase stmt) { : stmt_ = stmt; : } It looks like we don't use this anywhere. http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java: http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@28 PS11, Line 28: interfaces Nit: interface http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@31 PS11, Line 31: parser and the : * Calcite parser. Nit: Can we use a word other than "parser"? http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java File fe/src/main/java/org/apache/impala/service/CompilerFactory.java: http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@36 PS11, Line 36: /** : * Create a ParsedStatement class which does the parsing of the query. : */ : public ParsedStatement createParsedStatement(TQueryCtx queryCtx) throws ImpalaException; Down the road, frontend tests are going to be using this rather than creating the classes manually. So, there will be multiple signatures for this function (which might motivate an AbstractCompilerFactory). http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@54 PS11, Line 54: /** : * Return a fallback compiler factory given a planner context allowing the : * implementation to choose if it should fallback based on a query option or : * the type of exception thrown by the failed compiler factory. : */ : public CompilerFactory getFallbackFactory(TQueryOptions options, Exception e); I would prefer to keep fallback external to the compiler. I don't think the compiler has any extra information that helps the fallback decision. The compiler should be simple: Try to compile the statement, fail if it can't. Fallback is very simple today and can be expressed in 30 lines of code. I'd be reluctant to make it an abstract algorithm. http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java File fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java: http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@35 PS11, Line 35: * Factory implementation which creates Compiler implementation classes. Let's be clear that this is for the original planner. http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@37 PS11, Line 37: CompilerFactoryImpl If the Calcite version is going to be called CalciteCompilerFactory, should this be called OriginalCompilerFactory or RegularCompilerFactory? http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java File fe/src/test/java/org/apache/impala/common/FrontendFixture.java: http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@359 PS11, Line 359: ParsedStatement parsedStmt = new ParsedStatementImpl(stmt); I think we'll end up using a compiler factory to create this. This function may end up having a signature that passes in the compiler factory. There are times when we want to parse using the compiler under test, but there are also times where it must always be the regular compiler (e.g. when creating a view). http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@370 PS11, Line 370: CompilerFactory compilerFactory = new CompilerFactoryImpl(); I think the FrontendFixture will have the concept of the compiler under test. This would use the compiler under test rather than creating its own or a creating a parsed statement directly. The ParsedStatement needs to be produced by the same compiler factory as we use for analyzeAndAuthorize. -- 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 <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: Tue, 04 Mar 2025 03:58:26 +0000 Gerrit-HasComments: Yes
