-
Notifications
You must be signed in to change notification settings - Fork 93
feat(isthmus): mapping of positional scalar fns #610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fe03114 to
961bda4
Compare
bestbeforetoday
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional SQL function names I suggested testing already work with the code you had previously since they are Calcite SQL aliases and resolve to the same Calcite Rel. For example, both the SQL ENDS_WITH and ENDSWITH result in the Calcite ENDS_WITH Rel node. This means that your mapping from Calcite ENDS_WITH to Substrait ends_with works for both SQL variants with no special handling in our code required. It does mean that when converting Substrait ends_with back to SQL you always get ENDS_WITH, even if the input SQL used ENDSWITH, but that is fine since they are equivalent. Calcite will deal with mapping its Rel representation to the correct SQL form for a target execution engine.
In summary, the only changes needed were the additions to the testing coverage. The implementation code was fine so the extra changes made can be removed. See the inline comments with suggestions that (if they look OK to you) you should be able to apply to take you back to the minimal changes required.
-Added mappings for
STARTS_WITH,ENDS_WITH,CONTAINSandPOSITIONtoFunctionMappings.java-Added extra libraries to enable the included operators in
SubstraitOperatorTable.java-Added passing tests to
StringFunctionTest.javaincludingSTARTSWITH,ENDSWITH,STRPOSand literals