[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The ImpalaOperatorTable is used at validation time to check for existence of the function name for Impala. At first, it will check Calcite operators for the existence of the function name (A TODO, IMPALA-13096, is that we need to remove non-supported names from the parser file). It is preferable to use the Calcite Operator since Calcite does some optimizations based on the Calcite Operator class. If the name is not found within the Calcite Operators, a check is done within the BuiltinsDb (TODO: IMPALA-13095 handle UDFs) for the function. If found, and SqlOperator class is generated on the fly to handle this function. The validation process for Calcite includes a call into the operator method "inferReturnType". This method will validate that there exists a function that will handle the operands, and if so, return the "return type" of the function. In this commit, we will assume that the Calcite operators will match Impala functionality. In later commits, there will be overrides where we will use Impala validation for operators where Calcite's validation isn't good enough. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Reviewed-on: http://gerrit.cloudera.org:8080/21357 Reviewed-by: Michael Smith Tested-by: Impala Public Jenkins --- M java/calcite-planner/src/main/codegen/config.fmpp M java/calcite-planner/src/main/codegen/templates/Parser.jj A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M testdata/workloads/functional-query/queries/QueryTest/calcite.test 15 files changed, 962 insertions(+), 14 deletions(-) Approvals: Michael Smith: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 16
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 15: Verified+1 -- 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: 15 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Fri, 07 Jun 2024 00:27:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 15: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/21357/10/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/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@69 PS10, Line 69: this.savedFunction_ = fn; > Done Ack http://gerrit.cloudera.org:8080/#/c/21357/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java: http://gerrit.cloudera.org:8080/#/c/21357/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@76 PS10, Line 76: } > So this is more of a TODO if anything. For the most part, a function name s Ack -- 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: 15 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Thu, 06 Jun 2024 23:32:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 15: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10692/ DRY_RUN=true -- 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: 15 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Thu, 06 Jun 2024 19:14:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 14: Code-Review+1 Let's rebase and get a precommit run. I think we can go ahead with this. -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Thu, 06 Jun 2024 01:41:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 14: (1 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 : * > Starting to think about this a little more... If we implement custom immutable Exprs that initialize things in the constructor and have an empty analyzeImpl(), that's fine with me. I didn't intend my proposal to be incompatible with that, but it still requires us to implement custom classes for the Calcite planner. My best guess is that it would be a lot of work to modify the regular Impala Exprs to get rid of resetAnalysisState() completely. Maybe we can crack down on locations that require resetAnalysisState() along the path we use for the Calcite planner. -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Thu, 06 Jun 2024 01:40:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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: (1 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 : */ > I'll address this as a follow-up. Starting to think about this a little more... Yeah, I guess I'm not sure I like the post analysis solution. I would rather do something within the Expr class to remove the "resetAnalysisState" method. This is a mutable expression and has caused me major grief on some bugs in the past. It just feels wrong to me to have an analyzeImpl method that mutates the object. I think a general redesign should be done, as I mentioned in the previous post. So it leaves me at a crossroads. Do I implement it the way you suggested which is slightly more in line with the current way of doing things but makes the code more vulnerable to problems? Or do I address the major issue which is a major change to the system? Chances are I probably do it the way you suggested and that causes me a bit of pain. But we can discuss this in a later code review. -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 05 Jun 2024 23:13:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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: (1 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 : */ > The part where we immediately analyze as part of the constructor makes for I'll address this as a follow-up. To be honest though, I don't really like that solution much either because it still doesn't get at the core problem that the Expr object should be immutable. I think a much broader rewrite should be done. I can go ahead and make this change, but it's still not the right thing, imo. -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 05 Jun 2024 23:03:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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: (1 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 : */ > Heh, yeah, you got me thinking about this too because this changed from its The part where we immediately analyze as part of the constructor makes for complicated exception handling. RexVisitor doesn't support exceptions, so it adds complication to handle them under those circumstances. I can't really explain why it is necessary. Let me sketch out an alternative: 1. Construct the whole Expr tree without analyzing it 2. Any errors that happen during this process are not usually actionable by the end user. It's good to have a descriptive error message, but it doesn't mean there is something wrong with the SQL. I think that it is ok for this code to throw subclasses of RuntimeException or use Preconditions.checkState() with a good explanation. 3. When we get the Expr tree back in CreateExprVisitor::getExpr(), we call analyze() on the root node, which does a recursive analysis of the whole tree. 4. The special Expr classes don't run analyze() in the constructor, don't keep a reference to the Analyzer, and don't override resetAnalysisState(). They override analyzeImpl() and they should be idempotent. The clone constructor should not need to do anything special, just do a deep copy. I don't want to bog down this review. If we want to address this as a followup, I can live with that, but I don't want us to go too far down this road. (Or if we have a good explanation for why it is necessary, then we can write a good comment and move on.) -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 05 Jun 2024 21:05:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16275/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 05 Jun 2024 17:26:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#14). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The ImpalaOperatorTable is used at validation time to check for existence of the function name for Impala. At first, it will check Calcite operators for the existence of the function name (A TODO, IMPALA-13096, is that we need to remove non-supported names from the parser file). It is preferable to use the Calcite Operator since Calcite does some optimizations based on the Calcite Operator class. If the name is not found within the Calcite Operators, a check is done within the BuiltinsDb (TODO: IMPALA-13095 handle UDFs) for the function. If found, and SqlOperator class is generated on the fly to handle this function. The validation process for Calcite includes a call into the operator method "inferReturnType". This method will validate that there exists a function that will handle the operands, and if so, return the "return type" of the function. In this commit, we will assume that the Calcite operators will match Impala functionality. In later commits, there will be overrides where we will use Impala validation for operators where Calcite's validation isn't good enough. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- M java/calcite-planner/src/main/codegen/config.fmpp M java/calcite-planner/src/main/codegen/templates/Parser.jj A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M testdata/workloads/functional-query/queries/QueryTest/calcite.test 15 files changed, 962 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/21357/14 -- 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: newpatchset Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 14
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 05 Jun 2024 17:05:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 13:
(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 fixed this up a little. Added a comment. I also deleted the resetAnalys
I'm still wrapping my head around the always-analyzed concept.
My understanding of things:
1. The Impala code that consumes the plan assumes that everything has already
been analyzed (and has generated the associated state), so at some point the
Exprs we generate need to be analyzed before it gets to the Impala code.
2. Calcite is providing information that eliminates the need to use pieces of
the Impala analysis code. We override analyzeImpl() to simplify what needs to
be done (or do additional steps to incorporate that extra info).
3. Impala code will still be manipulating these Exprs, so they need to handle
being cloned or having resetAnalysisState() called.
Hopefully that is relatively close.
Some pieces I don't understand:
1. How important is the always-analyzed aspect of this? It seems like we need
to run analysis before passing these over to the Impala code. Is it crucial
that analysis happen immediately as part of the constructor? One could imagine
building the whole Expr tree and then calling analyze() on the root and letting
it recurse normally.
2. If we don't need to run analyze() in the constructor, maybe we wouldn't need
to run it in resetAnalysisState() either. That would get us pretty close to
eliminating the reference to the Analyzer.
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: @Override
: protected void resetAnalysisState() {
: try {
: // Need to save the type before resetting.
: Type savedType = type_;
: super.resetAnalysisState();
: this.analyze(null);
: uncheckedCastTo(savedType);
: } catch (AnalysisException e) {
: //TODO: IMPALA-13097: Don't throw runtime exception
: throw new RuntimeException("Exception reanalyzing
expression.", e);
: }
: }
Is it possible to eliminate this override, similar to how we eliminated the one
in AnalyzedFunctionCallExpr?
--
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: 13
Gerrit-Owner: Steve Carlin
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Steve Carlin
Gerrit-Comment-Date: Wed, 05 Jun 2024 01:04:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16270/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 13 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Tue, 04 Jun 2024 22:36:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Steve Carlin
Gerrit-Comment-Date: Tue, 04 Jun 2024 22:13:51 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#13). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The ImpalaOperatorTable is used at validation time to check for existence of the function name for Impala. At first, it will check Calcite operators for the existence of the function name (A TODO, IMPALA-13096, is that we need to remove non-supported names from the parser file). It is preferable to use the Calcite Operator since Calcite does some optimizations based on the Calcite Operator class. If the name is not found within the Calcite Operators, a check is done within the BuiltinsDb (TODO: IMPALA-13095 handle UDFs) for the function. If found, and SqlOperator class is generated on the fly to handle this function. The validation process for Calcite includes a call into the operator method "inferReturnType". This method will validate that there exists a function that will handle the operands, and if so, return the "return type" of the function. In this commit, we will assume that the Calcite operators will match Impala functionality. In later commits, there will be overrides where we will use Impala validation for operators where Calcite's validation isn't good enough. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- M java/calcite-planner/src/main/codegen/config.fmpp M java/calcite-planner/src/main/codegen/templates/Parser.jj A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M testdata/workloads/functional-query/queries/QueryTest/calcite.test 15 files changed, 971 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/21357/13 -- 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: newpatchset Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 13
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Steve Carlin
Gerrit-Comment-Date: Tue, 04 Jun 2024 18:27:41 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16235/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Tue, 28 May 2024 22:44:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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: (4 comments) http://gerrit.cloudera.org:8080/#/c/21357/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21357/11//COMMIT_MSG@10 PS11, Line 10: Only basic functions will work > "basic" is a bit vague here - is my understanding correct that all Impala b Heh... So there are gonna be some functions that aren't gonna work because of some special case scenarios. I've experienced this when I tried to get tpcds to work and some things didn't work through this mechanism. You'll see some of those things are commits that will come shortly. So the idea of using the word "basic" was to mean that this will support functions that don't require special case logic. I know I'm being a little handwavy here. This isn't meant to be supported for general use yet. I suppose I would rather get in a a general support for "most" functions first and then work on the special cases. One of the goals is to get tpcds working as quickly as possible so that we can do benchmarks. Then we can start making sure all the functions work and provide a test framework that tests all the functions. I suppose it's debatable which way to do this first? http://gerrit.cloudera.org:8080/#/c/21357/11//COMMIT_MSG@18 PS11, Line 18: parser > typo Done http://gerrit.cloudera.org:8080/#/c/21357/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java: http://gerrit.cloudera.org:8080/#/c/21357/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@72 PS11, Line 72: operatorList, nameMatche > What is the exceptions if this is false? To we expect operatorList to be em I added a preconditions check that it should be <= 1. There theoretically shouldn't be more than 1 because that would be 2 functions matched. Calcite allows that to happen and then kinda spits up when it does. So yeah, we should capture that and spit up right away rather than wait for Calcite to do it. And then handle it here in the code if it does happen. If it is zero, that means that it wasn't found in the Calcite operators and that's why we go on to check the Builtin functions for the name match. http://gerrit.cloudera.org:8080/#/c/21357/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/21357/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test@122 PS11, Line 122: no need to be extensive about testing in this file. > It would be nice to add basic coverage for types, so for each type T a func As I mentioned in a different comment. I'm kinda holding off on doing general function testing. I want to make sure things work in general though. There will be more testing done in later commits for sure. -- 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 Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Tue, 28 May 2024 22:19:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#12). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The ImpalaOperatorTable is used at validation time to check for existence of the function name for Impala. At first, it will check Calcite operators for the existence of the function name (A TODO, IMPALA-13096, is that we need to remove non-supported names from the parser file). It is preferable to use the Calcite Operator since Calcite does some optimizations based on the Calcite Operator class. If the name is not found within the Calcite Operators, a check is done within the BuiltinsDb (TODO: IMPALA-13095 handle UDFs) for the function. If found, and SqlOperator class is generated on the fly to handle this function. The validation process for Calcite includes a call into the operator method "inferReturnType". This method will validate that there exists a function that will handle the operands, and if so, return the "return type" of the function. In this commit, we will assume that the Calcite operators will match Impala functionality. In later commits, there will be overrides where we will use Impala validation for operators where Calcite's validation isn't good enough. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- M java/calcite-planner/src/main/codegen/config.fmpp M java/calcite-planner/src/main/codegen/templates/Parser.jj A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M testdata/workloads/functional-query/queries/QueryTest/calcite.test 15 files changed, 974 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/21357/12 -- 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: newpatchset Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 12
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/21357/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21357/11//COMMIT_MSG@10 PS11, Line 10: Only basic functions will work "basic" is a bit vague here - is my understanding correct that all Impala builtin scalar functions will work with this commit if the argument types match completely? As ImpalaOperatorTable will do a lookup in the builtin db and RexCallConverter will do the same through FunctionResolver, I don't see why a function would not work. http://gerrit.cloudera.org:8080/#/c/21357/11//COMMIT_MSG@18 PS11, Line 18: paresr typo http://gerrit.cloudera.org:8080/#/c/21357/11/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/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java@28 PS11, Line 28: L typo http://gerrit.cloudera.org:8080/#/c/21357/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java: http://gerrit.cloudera.org:8080/#/c/21357/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@72 PS11, Line 72: operatorList.size() == 1 What is the exceptions if this is false? To we expect operatorList to be empty, or there can be multiple elements? If operatorList is expected to contain 1 or 0 elements, then there could be a check for this. http://gerrit.cloudera.org:8080/#/c/21357/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/21357/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test@122 PS11, Line 122: no need to be extensive about testing in this file. It would be nice to add basic coverage for types, so for each type T a function that returns T and a function that gets T as argument. Is this feasible with the current functions set? -- 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: 11 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Tue, 21 May 2024 12:56:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16180/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 11 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Fri, 17 May 2024 19:59:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#11). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The ImpalaOperatorTable is used at validation time to check for existence of the function name for Impala. At first, it will check Calcite operators for the existence of the function name (A TODO, IMPALA-13096, is that we need to remove non-supported names from the paresr file). It is preferable to use the Calcite Operator since Calcite does some optimizations based on the Calcite Operator class. If the name is not found within the Calcite Operators, a check is done within the BuiltinsDb (TODO: IMPALA-13095 handle UDFs) for the function. If found, and SqlOperator class is generated on the fly to handle this function. The validation process for Calcite includes a call into the operator method "inferReturnType". This method will validate that there exists a function that will handle the operands, and if so, return the "return type" of the function. In this commit, we will assume that the Calcite operators will match Impala functionality. In later commits, there will be overrides where we will use Impala validation for operators where Calcite's validation isn't good enough. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- M java/calcite-planner/src/main/codegen/config.fmpp M java/calcite-planner/src/main/codegen/templates/Parser.jj A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M testdata/workloads/functional-query/queries/QueryTest/calcite.test 15 files changed, 972 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/21357/11 -- 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: newpatchset Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 11
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/21357/10/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/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@69 PS10, Line 69: //TODO: Don't throw runtime exception > If you're planning to leave TODOs in when we commit, please add the JIRA ti Done http://gerrit.cloudera.org:8080/#/c/21357/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java: http://gerrit.cloudera.org:8080/#/c/21357/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@76 PS10, Line 76: // There shouldn't be more than one opName with our usage, so throw an exception > Why is that? How is "our usage" unique? So this is more of a TODO if anything. For the most part, a function name should have one operator. When I was researching this months ago, I think I saw situations where some of the other parameters to this method (ie. category, syntax) can be used with opName. I don't think we care about that. I don't want to say I'm 100% sure though. So I put in this exception to catch a case if it was missed. Another important note on top of this: the Parser.jj still needs to be stripped a bit to only handle Impala functions. This is still a work in progress. So my theory is that once we strip out other noise, we won't have situations with 2 opNames for the same operator. Not sure I totally answered your question, but yeah, the short summary is that I want to catch situations in case this happens, but I don't think it will happen without usage. But we'll be able to catch it quickly if it does. -- 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: 10 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Fri, 17 May 2024 17:58:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/21357/10/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/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@69 PS10, Line 69: //TODO: Don't throw runtime exception If you're planning to leave TODOs in when we commit, please add the JIRA ticket where you plan to address them. http://gerrit.cloudera.org:8080/#/c/21357/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java: http://gerrit.cloudera.org:8080/#/c/21357/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@76 PS10, Line 76: // There shouldn't be more than one opName with our usage, so throw an exception Why is that? How is "our usage" unique? http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java: http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@119 PS2, Line 119:* Get the normalized RelDataType given an impala type. > Unused, deleted it. 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: 10 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Thu, 16 May 2024 21:09:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 8: Cleaned the code up in the most recent commit to use Impala's function resolving rather than a custom function resolving. -- 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: 8 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Tue, 14 May 2024 22:05:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#10). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The ImpalaOperatorTable is used at validation time to check for existence of the function name for Impala. At first, it will check Calcite operators for the existence of the function name (A TODO is that we need to remove non-supported names from the parser file). It is preferable to use the Calcite Operator since Calcite does some optimizations based on the Calcite Operator class. If the name is not found within the Calcite Operators, a check is done within the BuiltinsDb (TODO: handle UDFs) for the function. If found, and SqlOperator class is generated on the fly to handle this function. The validation process for Calcite includes a call into the operator method "inferReturnType". This method will validate that there exists a function that will handle the operands, and if so, return the "return type" of the function. In this commit, we will assume that the Calcite operators will match Impala functionality. In later commits, there will be overrides where we will use Impala validation for operators where Calcite's validation isn't good enough. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- M java/calcite-planner/src/main/codegen/config.fmpp M java/calcite-planner/src/main/codegen/templates/Parser.jj A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M testdata/workloads/functional-query/queries/QueryTest/calcite.test 15 files changed, 972 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/21357/10 -- 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: newpatchset Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 10 Gerrit-Owner: Steve Carlin
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java:
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@61
PS7, Line 61: abstract public class FunctionSignature {
> This is something I wish I had a good answer for and I'll have to figure th
Ok, I started implementing it using the Impala resolver, and I hit an issue...
In the FunctionDetailStatics module, there is a mapping of Calcite function
names to Impala function names. One of the unit tests that failed was with the
"cast" statement.
Impala knows each of its casting functions by a very specific name, like
"casttobigint", "casttoint", etc...
At resolving time for validation, we'd have to put in some special logic to
find the right function. This is one situation where I would prefer to use
Calcite's validation as opposed to Impala's. There's a bunch of work and
coding outside of the existing Impala logic to get the right type, and it would
be a "cast" specific logic.
Then, if we used the Impala resolver, we'd still have to get it back to a
Calcite operator. We could theoretically use a generic SqlKind.OTHER_FUNCTION
operator, but this would cause us to lose some Calcite optimization code
depending on the right type. So I don't think that would be right. Which
means we'd have to map each "casttobigint", etc.. function back to a Calcite
operator.
This is still doable, and perhaps this is the route to go. But then we also
need a translator after optimization which brings it back to the Impala
function from the Calcite operator. I was able to reuse the "function" logic
in its current iteration, but for the cast case, we would again need some
special logic to map Calcite to Impala.
I think the logic I encoded here seems pretty generic. I can't say I will be
able to get away with never special casing things. And indeed, the
FunctionDetailStatics object is a special case. But that's just a simple map
lumping all "cast" functions together, and I think the code is pretty small
there.
It's probably worth noting that "case" will prove to be a problem in the future
because the Impala signature is different from the Calcite signature. But
either way, special case logic will be needed for this.
I hope this makes sense. I'm open for discussion here. I can try to code it
the other way and see what it looks like, but I feel like the code here is
fairly compact? And generic?
--
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: 7
Gerrit-Owner: Steve Carlin
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Steve Carlin
Gerrit-Comment-Date: Tue, 07 May 2024 22:28:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java:
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@61
PS7, Line 61: abstract public class FunctionSignature {
> Impala has logic for this type of matching as part of its Function class. I
This is something I wish I had a good answer for and I'll have to figure this
out. This class has gone through some iterations for me and I started
development on it a few years back.
Upon looking at it, probably the reason I created it this way is because there
is no "Function" class here. It deals with types and name only. But of
course, the "Function" is just the types and the name (and the db, which is
included in the name).
I think it makes sense to keep this class to keep the relationship between
Calcite and Impala Functions, but perhaps the resolving portion should be fixed
up for reuse. I'll need to investigate this.
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java:
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@50
PS7, Line 50: public class ImpalaOperatorTable extends
ReflectiveSqlOperatorTable {
> If I'm understanding the Calcite code, this is primarily for sets of functi
Ok, this is gonna be a long-winded explanation. And perhaps there is another
approach we can take. But here is my thought process:
Calcite has a lot of built in logic when they resolve functions embedded in
their code. Their focus was to match standard SQL, not really to handle
non-standard SQL functions.
With Calcite's logic, they have an SqlKind built into their resolver, and their
different functions have different SqlKind values
https://javadoc.io/doc/org.apache.calcite/calcite-core/latest/index.html.
Embedded throughout their code are case statements that base their logic on the
SqlKind for the function. This is included with some of their optimization
rules.
So if we were to use purely our function resolver, we would have to map
individual functions to their appropriate SqlKind. I don't know if this is a
good thing or a bad thing, but I guess I went the "lazy" route and figured
let's let Calcite do the resolving and they will produce the right SqlKind
type. It also seems to fit in more with the Calcite way, imo if we ever
decided to standardize Impala a bit more?
Another potential issue is that Calcite and Impala might not necessarily be 1:1
on functions. There might be some standard SQL syntax that validates just fine
on Calcite, translates to Impala, but not in a 1:1 fashion. This could result
in some awkwardness at validation time. I'm not sure I have an example here,
so perhaps this isn't true.
You did ask about UDFs. While I haven't coded that yet, my gut instinct on how
this would be done is through Chained operator tables. Calcite allows lookups
to go through multiple order chained operator tables. Each UDF would be in its
own operator table object and chained on when doing the lookup.
You asked about the existing Impala lookup functionality, which I commented on
in another comment of yours.
--
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: 7
Gerrit-Owner: Steve Carlin
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Steve Carlin
Gerrit-Comment-Date: Tue, 07 May 2024 20:04:35 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16116/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 8 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Tue, 07 May 2024 17:03:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#8). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- M java/calcite-planner/src/main/codegen/config.fmpp M java/calcite-planner/src/main/codegen/templates/Parser.jj A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java:
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@61
PS7, Line 61: abstract public class FunctionSignature {
Impala has logic for this type of matching as part of its Function class. Is
there a way we could use that logic? Why or why not?
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@210
PS7, Line 210: It is possible for signatures to be equal when they have
: // a different number of arguments. For this reason, we will
return the same
: // hashcode based only on the first argument, if it exists.
This will result
: // in a couple extra collisions, but this cost should be small.
Ok, I get this now. This is about vararg functions. It's ok to hash the first
arg, because every vararg function has at least one non-vararg argument.
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java:
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@50
PS7, Line 50: public class ImpalaOperatorTable extends
ReflectiveSqlOperatorTable {
If I'm understanding the Calcite code, this is primarily for sets of functions
that rarely change. How do UDFs fit into that? How does the session's current
database interact with function lookups?
Have you considered reusing the existing Impala structures for looking up
functions? What does using the Calcite structure get us?
My mental model here is that function resolution should basically produce the
same results for Calcite vs regular Impala. Is that accurate or inaccurate?
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java:
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@125
PS7, Line 125: TPrimitiveType primitiveType =
impalaType.getPrimitiveType().toThrift();
: Type normalizedImpalaType = getImpalaType(primitiveType);
Nit: It may not be necessary to convert to TPrimitiveType. We have locations in
code that do a switch directly on PrimitiveType.
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@200
PS7, Line 200: -1
Nit: For these -1 precision values, can you use
RelDataType::PRECISION_NOT_SPECIFIED instead?
http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@320
PS7, Line 320: public static RelDataType createRelDataType(Type impalaType) {
What is the distinction between getRelDataType(Type impalaType) and
createRelDataType(Type impalaType)?
It looks like one is normalized and the other isn't for decimal? Let's try to
express that in the names so we can tell them apart.
--
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: 7
Gerrit-Owner: Steve Carlin
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Steve Carlin
Gerrit-Comment-Date: Tue, 07 May 2024 04:12:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16099/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 7 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Mon, 06 May 2024 17:12:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#7). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/templates/Parser.jj M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16071/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 6 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 01 May 2024 03:48:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/21357/4/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/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@34 PS4, Line 34: stateana > Nit: spelling? Is this supposed to be "that is always in an analyzed state" Done http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java: http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@219 PS5, Line 219: return Objects.hash(func, argTypes.get(0).getPrimitiveType().toString()); > How does this interact with inexact matching of types? The thing that pops I don't think this matters on the hashCode. While hashCode and equals need to work together, I think it's ok to have the hashCode match things that are a little more broad. It could theoretically cause an extra collision in a hash bucket, but if one is doing a HashMap.get(), it's still gonna hit the "equals" method. But I don't think there will really be too many extra collisions either because the first parameter is usually different for every function. If you're asking about how the code will work when the signature doesn't match and there is an implicit conversion? I have that coded and committed in a draft. That will be available shortly. http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java: http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java@32 PS5, Line 32: import org.apache.impala.analysis.SlotDescriptor; : import org.apache.impala.analysis.SlotRef; > Nit: These look unused? 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: 5 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 01 May 2024 03:23:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#6). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/templates/Parser.jj M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 5: (3 comments) I'm starting to wrap my head around this. I'll take another pass soon. General comment: We will have JUnit tests for the Calcite planner. The sooner we bite the bullet and start writing them, the better. http://gerrit.cloudera.org:8080/#/c/21357/4/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/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@34 PS4, Line 34: stateana Nit: spelling? Is this supposed to be "that is always in an analyzed state"? http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java: http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@219 PS5, Line 219: return Objects.hash(func, argTypes.get(0).getPrimitiveType().toString()); How does this interact with inexact matching of types? The thing that pops into my head is a worry that we could have inexact type matching of the first parameter. Is that a thing that could happen? http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java: http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java@32 PS5, Line 32: import org.apache.impala.analysis.SlotDescriptor; : import org.apache.impala.analysis.SlotRef; Nit: These look unused? -- 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: 5 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Wed, 01 May 2024 02:57:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16059/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 5 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Steve Carlin Gerrit-Comment-Date: Tue, 30 Apr 2024 04:39:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#5). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/templates/Parser.jj M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
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 2:
(2 comments)
Rebase is done now. Will add a better comment in a bit.
http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java:
http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@119
PS2, Line 119: public static List
createRelDataTypesForArgs(List impalaTypes) {
> How does this differ from createRelDataTypes at line 318?
Unused, deleted it.
http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@130
PS2, Line 130: public static RelDataType getRelDataType(Type impalaType) {
> I'm not sure why this and createRelDataType both exist. They're very simila
The getRelDataType deals with datatypes where the precision and scale might not
be known. The "normalized" comment probably wasn't clear enough, and I know
"get" and "create" can be confusing. "get" was meant to convey that these
datatypes already exist in a map, whereas "create" will grab the precision and
scale from the Type passed in and "create" a new datatype.
I'll try to make the comment clearer.
--
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: 2
Gerrit-Owner: Steve Carlin
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Steve Carlin
Gerrit-Comment-Date: Mon, 29 Apr 2024 15:15:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16049/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 4 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Mon, 29 Apr 2024 01:42:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#4). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/templates/Parser.jj M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Michael Smith has posted comments on this change. (
http://gerrit.cloudera.org:8080/21357 )
Change subject: IMPALA-12935: First pass on Calcite planner functions
..
Patch Set 2:
(2 comments)
Looks like this stack needs a rebase. I can't pull it locally either, I get an
"internal server error".
http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java
File
java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java:
http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@119
PS2, Line 119: public static List
createRelDataTypesForArgs(List impalaTypes) {
How does this differ from createRelDataTypes at line 318?
http://gerrit.cloudera.org:8080/#/c/21357/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@130
PS2, Line 130: public static RelDataType getRelDataType(Type impalaType) {
I'm not sure why this and createRelDataType both exist. They're very similar
except this one doesn't handle Decimal types.
--
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: 2
Gerrit-Owner: Steve Carlin
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Comment-Date: Fri, 26 Apr 2024 22:37:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16023/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 2 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Thu, 25 Apr 2024 21:33:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21357 to look at the new patch set (#2). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/templates/Parser.jj M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16022/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 1 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Thu, 25 Apr 2024 21:04:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Steve Carlin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21357 Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 --- A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/templates/Parser.jj M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M testdata/workloads/functional-query/queries/QueryTest/calcite.test 21 files changed, 1,747
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21195 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/16021/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/21195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8b57fab2a6b422e4a76b7b4d70fb75bbe0b81ad Gerrit-Change-Number: 21195 Gerrit-PatchSet: 9 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Thu, 25 Apr 2024 20:31:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Steve Carlin has abandoned this change. ( http://gerrit.cloudera.org:8080/21195 ) Change subject: IMPALA-12935: First pass on Calcite planner functions .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/21195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: If8b57fab2a6b422e4a76b7b4d70fb75bbe0b81ad Gerrit-Change-Number: 21195 Gerrit-PatchSet: 9 Gerrit-Owner: Steve Carlin Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-12935: First pass on Calcite planner functions
Hello Joe McDonnell, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21195 to look at the new patch set (#9). Change subject: IMPALA-12935: First pass on Calcite planner functions .. IMPALA-12935: First pass on Calcite planner functions This commit handles the first pass on getting functions to work through the Calcite planner. Only basic functions will work with this commit. Implicit conversions for parameters are not yet supported. Custom UDFs are also not supported yet. The functions are loaded in CalciteJniFrontend from the Impala "builtin" database. A "FunctionSignature" is created for each builtin function. Each function is loaded into Calcite's "ImpalaOperatorTable" object which is used in the validation process. Each Impala function maps to a Calcite "ImpalaOperator" object. For function names that are not an exact match with a Calcite operator, an entry is found in FunctionDetailStatics to do the conversion. When the Calcite validator tries to validate the function, it gets the return type through the "ImpalaOperator.inferReturnType()" method. In this method, a "FunctionSignatureForLookup" signature is created for looking up the stored signatures. The "Lookup" signature does not have a return type (yet), so the matching of signatures will be based on matching name and parameters. As mentioned above, implicit conversion is not yet supported in this commit; this will be added later. After validation is complete, the functions will be in a Calcite format. After the rest of compilation (relnode conversion, optimization) is complete, the function needs to be converted back into Impala form (the Expr object) to eventually get it into its thrift request. In this commit, all functions are converted into Expr starting in the ImpalaProjectRel, since this is the RelNode where functions do their thing. The RexCallConverter and RexLiteralConverter get called via the CreateExprVisitor for this conversion. Since Calcite is providing the analysis portion of the planning, there is no need to go through Impala's Analyzer object. However, the Impala planner requires Expr objects to be analyzed. To get around this, the AnalyzedFunctionCallExpr and AnalyzedNullLiteral objects exist which analyze the expression in the constructor. While this could potentially be combined with the existing FunctionCallExpr and NullLiteral objects, this fits in with the general plan to avoid changing "fe" Impala code as much as we can until much later in the commit cycle. Also, there will be other Analyzed*Expr classes created in the future, but this commit is intended for basic function call expressions only. One minor change to the parser is added with this commit. Calcite parser does not have acknowledge the "string" datatype, so this has been added here in Parser.jj and config.fmpp. Change-Id: If8b57fab2a6b422e4a76b7b4d70fb75bbe0b81ad --- A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionDetailStatics.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForLookup.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignatureForStorage.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggregateOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaHelperOperator.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java A java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaScalarOperator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp M java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/templates/Parser.jj M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java M java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java M
