[jira] [Commented] (CALCITE-5575) Incorrect priority of builtinFunctionCallMethods in Parser.jj

["Julian Hyde (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-5575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17699856#comment-17699856 ] 

Julian Hyde commented on CALCITE-5575:
--------------------------------------

I don't know whether there's a general-purpose solution for what should happen when there's a clash between the base and derived parser. But your proposed solution, to relying on the ordering - and how JavaCC handles ambiguous grammars - seems very fragile.

In this particular case, these functions should not have been added to the parser at all, but they were added because time frame is syntactically not an expression. If we can make time frame syntactically an expression then the parser changes can be backed out, and the validator can do the resolution (based on a {{{}SqlOperandTypeChecker{}}}) 

[~tanclary], Can you make sure that a bug is logged for that refactoring?

> Incorrect priority of builtinFunctionCallMethods in Parser.jj
> -------------------------------------------------------------
>
>                 Key: CALCITE-5575
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5575
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.34.0
>            Reporter: James Turton
>            Priority: Major
>             Fix For: 1.34.0
>
>
> CALCITE-5469 and others have introduced new date and time functions. In Parser.jj (line 6152) these new functions are handled _above_ the builtinFunctionCallMethods Freemarker loop that allows applications to add their own function defintions.
> {code:java}
>         node = DateTimeConstructorCall() { return node; }
>     |
>         node = DateDiffFunctionCall() { return node; }
>     |
>         node = DateTruncFunctionCall() { return node; }
>     |
>         node = TimestampAddFunctionCall() { return node; }
>     |
>         node = DatetimeDiffFunctionCall() { return node; }
>     |
>         node = TimestampDiffFunctionCall() { return node; }
>     |
>         node = TimestampDiff3FunctionCall() { return node; }
>     |
>         node = TimestampTruncFunctionCall() { return node; }
>     |
>         node = TimeDiffFunctionCall() { return node; }
>     |
>         node = TimeTruncFunctionCall() { return node; }
>     |
> <#list (parser.builtinFunctionCallMethods!default.parser.builtinFunctionCallMethods) as method>
>         node = ${method} { return node; }
>     |
> </#list>{code}
> This means (I think) that applications are effectively unable to override the new date and time function definitions with their own definitions. In particular, Drill cannot override the new three-parameter DATE_DIFF function in order to provide the parser method needed by its own two-parameter DATE_DIFF function resulting in a regression in the form of a broken DATE_DIFF function.
> If the above is correct then can the builtinFunctionCallMethods Freemarker loop move above these date and time functions? Could it even move above the special syntax functions higher up, for that matter?
> Incidentally I also noted in Parser.jj that the new date and time functions mentioned above are also handled seperately in JdbcFunctionCall which implements JDBC function call syntax. This makes me unsure whether application overrides provided in builtinFunctionCallMethods will take priority over these Calcite definitions when they are invoked with JDBC syntax.
>  



--
This message was sent by Atlassian Jira
(v8.20.10#820010)