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 14: (2 comments) 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 : * > I'm still wrapping my head around the always-analyzed concept. Heh, yeah, you got me thinking about this too because this changed from its initial design. So that comment is stale. I'm gonna hold off on changing the comment until the discussion here is complete because I think you may have some more input as to what I am about to write next. What makes it stale is the latest change. I removed resetAnalyzeImpl here. This winds up getting called in various spots in the code which is immediately followed by an "analyze" call, so there is a brief moment when the object will remain unanalyzed. I'm not sure I understand point 2, but one thing I will say: I do not like any design/code that involves mutating objects unless absolutely necessary. So "resetAnalyzeState()" is something I wish we could get rid of. Not sure how much work that would be in the current Impala analysis though. But perhaps we can refactor the Planner code so that it doesn't get called there. http://gerrit.cloudera.org:8080/#/c/21357/13/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java: http://gerrit.cloudera.org:8080/#/c/21357/13/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java@54 PS13, Line 54: // resetAnalyzedState will remove the type so we use the savedType : // from the constructor : uncheckedCastTo(savedType_); : } : } : : : : : : : : > Is it possible to eliminate this override, similar to how we eliminated the Done -- 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: 14 Gerrit-Owner: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@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: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Wed, 05 Jun 2024 17:05:24 +0000 Gerrit-HasComments: Yes
