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 <[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 22:28:52 +0000 Gerrit-HasComments: Yes
