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 18: (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 Done 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: Heh, yeah, I don't like any of them either, but you're right, we gotta do something to prevent bugs in the future How about a third option, very similar to #2, but at least it will help with debugging: Keep it final, but add a different non-final variable called "invalidated_" Set this after analysisResult. And then in the getTopLevelNode(), we can throw an exception if true. This will at least make it explicit while it's failing as opposed to just returning a null. Or is that too complicated? 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 > For now, let's keep the code simple. Down the road, we'll have better infor I kinda disagree with this one, but I'm not gonna argue about it. Thought this was a pretty slick way of doing it. The fallback wasn't external...it just passed back the CompilerFactory to use if it fails. However, I'm not sure what you're picturing here then as to how to trigger the fallback. We can't have any mention of Calcite in the main code (except for the class name through reflection). Were you picturing just having a boolean variable that says "allowsFallback" and then use the internal compilerFactory if it does? This code almost does the same thing...it just returns the internal compiler factory rather than hardcoding it. Or were you picturing the getTExecRequest to be an abstract method somewhere within the compilerFactory and the code is in the implementation? We'd have to pass Frontend into the method in that case, which I'm not sure I like. And that also makes fallback internal within the compiler. Or maybe I'm just missing something obvious. 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: the > We're using the ParsedStatement from the parser, but the AnalysisResult cou This needs fixing, 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: 18 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 23:39:49 +0000 Gerrit-HasComments: Yes
