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

Add missing support for numerical JDBC functions

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.12.0
    • Component/s: core
    • Labels:
      None

      Description

      Calcite advertises support for all numerical functions but only have implementations for some (ABS, EXP, LOG, LOG10, MOD, POWER)

      It would be a nice-to have to extend the support so that all functions are covered.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.12.0 (2017-03-24).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/17dc06fe . Thanks for the PR, Laurent Goujon !
          Hide
          laurentgo Laurent Goujon added a comment -

          I updated the pull request.

          Show
          laurentgo Laurent Goujon added a comment - I updated the pull request.
          Hide
          laurentgo Laurent Goujon added a comment -

          I'm almost done with your suggested changes, but unfortunately have been distracted. I should be able to update the PR over the week-end.

          Show
          laurentgo Laurent Goujon added a comment - I'm almost done with your suggested changes, but unfortunately have been distracted. I should be able to update the PR over the week-end.
          Hide
          julianhyde Julian Hyde added a comment -

          Laurent Goujon, Any update on this? I've just started committing fixes now that 1.11 is released, so this could go in when it is ready.

          Show
          julianhyde Julian Hyde added a comment - Laurent Goujon , Any update on this? I've just started committing fixes now that 1.11 is released, so this could go in when it is ready.
          Hide
          julianhyde Julian Hyde added a comment -

          Oops, I see from http://calcite.apache.org/docs/reference.html#keywords that FLOOR, CEIL and CEILING are already reserved. Sorry I misled you.

          I believe that means they should go in ReservedFunctionName(). If that breaks SqlAdvisorTest, just fix the expected output – it's quite a fragile test.

          Show
          julianhyde Julian Hyde added a comment - Oops, I see from http://calcite.apache.org/docs/reference.html#keywords that FLOOR, CEIL and CEILING are already reserved. Sorry I misled you. I believe that means they should go in ReservedFunctionName() . If that breaks SqlAdvisorTest , just fix the expected output – it's quite a fragile test.
          Hide
          laurentgo Laurent Goujon added a comment - - edited

          Small question about FLOOR and CEILING: in which list of tokens should these one be? Not adding to CommonNonReservedKeywords or ReservedFunctionNames obviously doesn't work ( {fn CEILING(...)} would fail parsing), but adding it to CommonNonReservedKeywords will make some of SqlAdvisorTest to fail because the list of suggestion would not have FLOOR or CEILING.

          Or maybe (probably?) I totally misunderstood what these lists are for?

          Show
          laurentgo Laurent Goujon added a comment - - edited Small question about FLOOR and CEILING : in which list of tokens should these one be? Not adding to CommonNonReservedKeywords or ReservedFunctionNames obviously doesn't work ( { fn CEILING(...) } would fail parsing), but adding it to CommonNonReservedKeywords will make some of SqlAdvisorTest to fail because the list of suggestion would not have FLOOR or CEILING . Or maybe (probably?) I totally misunderstood what these lists are for?
          Hide
          laurentgo Laurent Goujon added a comment -

          Thanks for your comments, it's very valuable. I'll update the patch accordingly

          Show
          laurentgo Laurent Goujon added a comment - Thanks for your comments, it's very valuable. I'll update the patch accordingly
          Hide
          julianhyde Julian Hyde added a comment -

          Thanks for this – much needed!

          A few review comments:

          • It seems overkill to have a whole class SqlPIContextVariable. I think you added it because you wanted to support the syntax PI rather than PI(). If so, could you instead repurpose SqlStringContextVariable as a base class (maybe you need to expose returnTypeInference as a constructor argument).
          • Is it really necessary for FLOOR and CEILING to become reserved?
          • Most of these functions should be callable without the JDBC fn prefix. (FLOOR AND CEILING are exceptions, because SQL already has FLOOR and CEIL.) Can you add tests for these? In SqlOperatorBaseTest would be fine. And add them to the "Arithmetic operators and functions" section in reference.md.
          • Please add one or two tests for calling these with null values, and negative tests (e.g. calling with boolean values). The operators are so similar, you don't need to test all operators, just one or two.
          • Remove the empty "Not implemented" section in reference.md.
          Show
          julianhyde Julian Hyde added a comment - Thanks for this – much needed! A few review comments: It seems overkill to have a whole class SqlPIContextVariable . I think you added it because you wanted to support the syntax PI rather than PI() . If so, could you instead repurpose SqlStringContextVariable as a base class (maybe you need to expose returnTypeInference as a constructor argument). Is it really necessary for FLOOR and CEILING to become reserved? Most of these functions should be callable without the JDBC fn prefix. (FLOOR AND CEILING are exceptions, because SQL already has FLOOR and CEIL.) Can you add tests for these? In SqlOperatorBaseTest would be fine. And add them to the "Arithmetic operators and functions" section in reference.md. Please add one or two tests for calling these with null values, and negative tests (e.g. calling with boolean values). The operators are so similar, you don't need to test all operators, just one or two. Remove the empty "Not implemented" section in reference.md.
          Hide
          laurentgo Laurent Goujon added a comment -

          Attaching manually the pull request: https://github.com/apache/calcite/pull/342

          (I thought ASF had a bot dedicated for it)

          travis-ci builds ailed but it seems to be a memory issue with the elasticsearch module (java heap space).

          Show
          laurentgo Laurent Goujon added a comment - Attaching manually the pull request: https://github.com/apache/calcite/pull/342 (I thought ASF had a bot dedicated for it) travis-ci builds ailed but it seems to be a memory issue with the elasticsearch module (java heap space).
          Hide
          laurentgo Laurent Goujon added a comment -

          I should be able to post an initial version shortly

          Show
          laurentgo Laurent Goujon added a comment - I should be able to post an initial version shortly

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              laurentgo Laurent Goujon
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development