Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-216

Inconsistent use of provided operator table causes inability to add aggregate functions

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      Description

      I found this problem when trying to provide an Apache Drill custom aggregate function. Optiq complains that the field referenced inside the aggregate function isn't part of the grouping. Clearly, Optiq isn't recognizing that it is an Aggregate function despite extending from SqlAggFunction.

      The Problem: When using frameworks and calling getPlanner(... SqlStdOperatorTable ...) the framework only uses this table for final function resolution. However, initial resolution done in SqlAbstractParserImpl uses the default SqlStdOperatorTable. If this initial resolution fails, the parser creates a SqlBasicCall (which is not an aggregator). Query validation will then fail if the unknown function is used where a grouping context is needed.

      At least two options exist:

      • modify AbstractParserImpl to leverage the same operator table as provided via the getPlanner() method rather than initializing its own.
      • update the validator to try to resolve the function before running AggChecker.

      Additional Notes:

      • The planner interface is providing requiring a subclass of SqlStdOperatorTable rather than the SqlOperatorTable interface. This makes overriding unnecessarily misleading.
      • I believe this can be addressed by creating a custom SqlParser using the recently merged parser factory issue, however that seems like a hack.

      Assigning to Julian to get his feedback on approach.

      ---------------- Imported from GitHub ----------------
      Url: https://github.com/julianhyde/optiq/issues/216
      Created by: jacques-n
      Labels: bug,
      Assignee: julianhyde
      Created at: Sun Mar 30 21:44:55 CEST 2014
      State: closed

        Activity

        Hide
        github-import GitHub Import added a comment -

        [Date: Mon Mar 31 00:26:08 CEST 2014, Author: julianhyde]

        Your analysis is spot-on. I prefer option 2 (update the validator). Option 1 violates the purpose of having an operator table in the parser – perform a SIMPLE initial pass to prevent SQL built-in functions from being quoted when converted back to SQL. The operator table in the parser must only contain SQL built-ins. Also, as general principle, a parser shouldn't be doing semantic checking; resolving operator overloading and type-checking come into that category.

        The validator should be doing the resolution. Since it apparently isn't, we're missing a step in the validator.

        Next step is to write a test case in PlannerTest. Make sure that validation succeeds with a user-defined aggregate function.

        Full user-defined aggregates in an Optiq JSON model, implemented using `AggImplementor2`, is a different level of ambition.

        Show
        github-import GitHub Import added a comment - [Date: Mon Mar 31 00:26:08 CEST 2014, Author: julianhyde ] Your analysis is spot-on. I prefer option 2 (update the validator). Option 1 violates the purpose of having an operator table in the parser – perform a SIMPLE initial pass to prevent SQL built-in functions from being quoted when converted back to SQL. The operator table in the parser must only contain SQL built-ins. Also, as general principle, a parser shouldn't be doing semantic checking; resolving operator overloading and type-checking come into that category. The validator should be doing the resolution. Since it apparently isn't, we're missing a step in the validator. Next step is to write a test case in PlannerTest. Make sure that validation succeeds with a user-defined aggregate function. Full user-defined aggregates in an Optiq JSON model, implemented using `AggImplementor2`, is a different level of ambition.
        Hide
        github-import GitHub Import added a comment -

        [Date: Mon Mar 31 01:08:02 CEST 2014, Author: jacques-n]

        Given the strategy of updating the validator, this brings up a second question (which I initially planned on managing via our own operator table. However, since your desire is to keep that operator fixed in the Abstract parser, we're having an issue where when you applied this change:

        https://github.com/julianhyde/optiq/commit/0449cc25502744bf8fa90fc8f5c17417d6fd27db#diff-d630911d7de9a0abdb93777417a5c241

        Operator resolution no longer works if a user writes sum() instead of SUM(). This is despite the fact that we are stating that we are case insensitive in the Lex call when we create a planner. What is your expected behavior when a user types lowercase sum?

        I can try to generate a failing test case. I'll need your help to figure out the validator reordering.

        Show
        github-import GitHub Import added a comment - [Date: Mon Mar 31 01:08:02 CEST 2014, Author: jacques-n ] Given the strategy of updating the validator, this brings up a second question (which I initially planned on managing via our own operator table. However, since your desire is to keep that operator fixed in the Abstract parser, we're having an issue where when you applied this change: https://github.com/julianhyde/optiq/commit/0449cc25502744bf8fa90fc8f5c17417d6fd27db#diff-d630911d7de9a0abdb93777417a5c241 Operator resolution no longer works if a user writes sum() instead of SUM(). This is despite the fact that we are stating that we are case insensitive in the Lex call when we create a planner. What is your expected behavior when a user types lowercase sum? I can try to generate a failing test case. I'll need your help to figure out the validator reordering.
        Hide
        github-import GitHub Import added a comment -

        [Date: Mon Mar 31 02:52:13 CEST 2014, Author: julianhyde]

        Please log a bug with a test case for the `sum` versus `SUM` issue.

        Show
        github-import GitHub Import added a comment - [Date: Mon Mar 31 02:52:13 CEST 2014, Author: julianhyde ] Please log a bug with a test case for the `sum` versus `SUM` issue.

          People

          • Assignee:
            Unassigned
            Reporter:
            github-import GitHub Import
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development