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 21: (5 comments) Ok, think I got all of this done. Wanted to put in here as a last comment a potential future change though. In the original patch, I put in a generic way of handling fallback. After discussion, I was pointed out a flaw in the design. The getFallbackFactory should not have been in the interface since there could theoretically be multiple planners. The framework should have been in control of that. But I think I went overboard in moving things out of the Calcite planner for the record. If a statement is a non-select (e.g. create table), the code to fallback is in the framework. THis is really specific to the Calcite planner, and the logic should be in there, not in the framework. The framework really shouldn't be aware of the Calcite planner, imo. Perhaps this is future work, but this has gotten a +2 now, so I will be committing this soon. http://gerrit.cloudera.org:8080/#/c/22306/21/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/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@91 PS21, Line 91: private final ParsedStatement parsedStmt_; > nit: it would be good to add a comment here about why both ParsedStatement Done http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@106 PS21, Line 106: : null; > Having a null value for stmt_ is not a good state as a lot of code derefere I put it in the comment, but this will be null for the Calcite planner. The StatementBase is legacy planner specific. As you mentioned, I kept this for convenience. Just looking at lines 111 and below, the code review would have been bigger (well maybe not that much bigger) if I dereferenced parsedStmt all the time, so I figured this would be better. http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@497 PS21, Line 497: //XXX: returnsAtMostOneRow should be in interface > Let's file a JIRA for this. It's low priority as this seems like an edge ca Done http://gerrit.cloudera.org:8080/#/c/22306/21/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/21/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@22 PS21, Line 22: import org.apache.impala.analysis.AnalysisContext.AnalysisDriverImpl; : import org.apache.impala.analysis.AnalysisContext.AnalysisResult; : import org.apache.impala.analysis.AnalysisDriver; : import org.apache.impala.analysis.Parser; : import org.apache.impala.analysis.QueryStmt; : import org.apache.impala.analysis.TableName; : import org.apache.impala.analysis.StatementBase; : import org.apache.impala.analysis.StmtMetadataLoader; : import org.apache.impala.authorization.AuthorizationFactory; : import org.apache.impala.common.ImpalaException; : import org.apache.impala.thrift.TQueryCtx; : import org.apache.impala.thrift.TQueryOptions; : import org.apache.impala.thrift.TResultSetMetadata; : import org.apache.impala.thrift.TStmtType; : : import static org.apache.impala.analysis.ToSqlOptions.SHOW_IMPLICIT_CASTS; > Nit: Let's take a pass at cleaning up these imports (e.g. AnalysisDriverImp Done http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/service/Frontend.java@3522 PS21, Line 3522: private CompilerFactory getCompilerFactory(PlanCtx ctx) { : return new CompilerFactoryImpl(); : } > Nit: I think this is unused now Done -- 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: 21 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: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Wed, 26 Mar 2025 00:46:07 +0000 Gerrit-HasComments: Yes
