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 17: (4 comments) http://gerrit.cloudera.org:8080/#/c/22306/17/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/17/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@549 PS17, Line 549: Nit: stray whitespace http://gerrit.cloudera.org:8080/#/c/22306/17/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/17/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@45 PS17, 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 stmt_ ; We've got two choices here: 1. ParsedStatementImpl can allow modifications to stmt_. This means that the original ParsedStatement from the parser is same as the one on AnalysisResult. They point to the same object, but it can change during analysis. 2. ParsedStatementImpl can't be modified. AnalysisResult creates a new ParsedStatementImpl when it rewrites a query and has a new StatementBase. There are two different ParsedStatements that could be slightly different. One is directly from the parser. The other is on the AnalysisResult with the rewritten statement. Right now, we're doing #2, so we need to be careful about which ParsedStatement we are using. Maybe #1 is easier to reason about as it doesn't end up with two different objects. They both have downsides, so I'm not sure which is better. If we go with #1, then this class will have some way to modify stmt_ and this will not be final. If we go with #2, then we can drop the comment about this not being final. We might want to take additional steps to avoid problems with the two different ParsedStatement objects. e.g. Null out the ParsedStatement from the parser once we get the AnalysisResult back, forcing subsequent code to use the ParsedStatement from the AnalysisResult. 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@54 PS11, Line 54: public SingleNodePlannerIntf createSingleNodePlanner(PlannerContext ctx) : throws ImpalaException; : : /** : * Return a fallback compiler factory given a planner context allowing the : * implementation to choose if it should fallback based on a query option or > Down the road, I was thinking that eventually we are going to treat Calcite For now, let's keep the code simple. Down the road, we'll have better information about what exactly we want to do for fallback and why. If we need something like this in the future, we will introduce it. http://gerrit.cloudera.org:8080/#/c/22306/17/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/17/fe/src/main/java/org/apache/impala/service/Frontend.java@2952 PS17, Line 2952: parsedStmt We're using the ParsedStatement from the parser, but the AnalysisResult could have a different ParsedStatement that was rewritten. I don't know if that makes a difference (hopefully not). -- 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: 17 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: Mon, 17 Mar 2025 22:00:59 +0000 Gerrit-HasComments: Yes
