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 <[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, 07 May 2024 20:04:35 +0000 Gerrit-HasComments: Yes
