Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 )
Change subject: IMPALA-12935: First pass on Calcite planner functions ...................................................................... Patch Set 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/codegen/templates/Parser.jj File java/calcite-planner/src/main/codegen/templates/Parser.jj: http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/codegen/templates/Parser.jj@6003 PS12, Line 6003: /* Added Impala code to handle String type */ Nit: For this particular type of comment, it looks like the file uses // style comments and indents them to align with the code. (Same for the other /**/ comments below) http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/codegen/templates/Parser.jj@6017 PS12, Line 6017: /* Added Impala code to handle String type */ See other comment. http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/codegen/templates/Parser.jj@6026 PS12, Line 6026: /* Added Impala code to handle String type */ See other comment. http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@33 PS12, Line 33: /** : * A FunctionCallExpr that is always in an analyzed state : */ In some comment, we need to lay out why these always analyzed classes exist. What makes something require the always analyzed version? What is happening that requires resetAnalysisState() and how do the overrides fix it? http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@106 PS12, Line 106: @Override : public void resetAnalysisState() { : try { : // The parent FunctionCallExpr sets the fn_ to null and sets it back again : // in analysisImpl. Since we override analysisImpl, we save the Function here : // before resetting and set it again. : Function savedFunction = fn_; : super.resetAnalysisState(); : fn_ = savedFunction; : this.analyze(analyzer_); : } catch (AnalysisException e) { : throw new RuntimeException("Exception reanalyzing expression.", e); : } : } >From a mechanics point of view, something resets the analysis state. I assume >that means it will call analyze again (and run analyzeImp()). Is that right? Is it possible that we could save the function, run super.resetAnalysisState(), restore saved function, but then not run analyze() here? http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java: http://gerrit.cloudera.org:8080/#/c/21357/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java@47 PS12, Line 47: /** : * CreateExprVisitor will generate Impala expressions for function calls and literals. : */ If I'm reading this right, this is basically a bottom-up tree traversal? -- To view, visit http://gerrit.cloudera.org:8080/21357 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 12 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[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: Tue, 04 Jun 2024 18:27:41 +0000 Gerrit-HasComments: Yes
