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 <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 28 May 2024 22:19:32 +0000 Gerrit-HasComments: Yes
