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 <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Tue, 21 May 2024 12:56:01 +0000 Gerrit-HasComments: Yes
