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 13: (12 comments) This should still be reviewed after https://gerrit.cloudera.org/#/c/22591/ 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: return analyzer_; > I thought of 4 different designs I could go with in which I have a clear ra As mentioned in the general comment, this design is changed in IMPALA-13481 http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@586 PS11, Line 586: y { > Style-wise, we only use the analysisResult_ field on ctx_ and we use it in Same as comment above 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: referenced > Nit: referenced Done 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 final StatementBase s > If I'm understanding this right, we don't use setTopLevelNode() anywhere, s Done http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@78 PS11, Line 78: @Override : public boolean isExplain() { : return stmt_.isExplain(); : } > It looks like we don't use this anywhere. Done 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: interface > Nit: interface Done http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@31 PS11, Line 31: planner and the : * Calcite planner > Nit: Can we use a word other than "parser"? Done 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: public interface CompilerFactory { : : /** : * Create a ParsedStatement class which does the parsing of the query. > Down the road, frontend tests are going to be using this rather than creati Ack. We can discuss further on that commit, I don't mind re-changing the API. I think it's possible without changing it, but I guess it depends on how much restructuring we want to do. 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: > Let's be clear that this is for the original planner. Ok, changed comment. Looking at the comment below, maybe we should call it "Original". Idk, even that seems off. http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@37 PS11, Line 37: plementation of the > If the Calcite version is going to be called CalciteCompilerFactory, should I struggled with the name here. Regular doesn't seem right to me. Original maybe works? I'm not sure. I liked going with just "Impl" because it's the standard implementation. But I'm definitely open to changing this. 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 Ack. Let's discuss this further when that commit comes in. 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 tes Ack. Let's discuss this further when that commit comes in. -- 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: 13 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: Sat, 08 Mar 2025 18:12:27 +0000 Gerrit-HasComments: Yes
