Steve Carlin 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 // st Done 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. Done 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. Done 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 I fixed this up a little. Added a comment. I also deleted the resetAnalysisState override which is no longer needed after the latest change, since the override of analyzeImpl() does everything that's needed. 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 assu deleted because it's no longer necessary 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? Yeah, it has to create the parameter Expr objects first and then the current function. -- 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 22:13:51 +0000 Gerrit-HasComments: Yes
