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

Add a method to SqlOperatorBinding to determine whether operand is a literal

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: core
    • Labels:
      None

      Description

      When Drill is doing type inference, a decimal literal can be treated as double in some cases.

      However, SqlOperatorBinding has not had a method to tell the caller if an operand is literal or not. This JIRA proposes adding such a helper method.

        Activity

        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        Julian Hyde Can you please help review? Thanks.

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - Julian Hyde Can you please help review? Thanks.
        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - PR: https://github.com/apache/calcite/pull/222
        Hide
        julianhyde Julian Hyde added a comment -

        The code looks OK.

        Can you add a test case? For instance, does Drill's case treating a decimal literal as a double apply to Calcite?

        Can you review the javadoc related existing methods getOperandLiteralValue, isOperandNull. These methods treat casted literals as if they were literals. Should isOperandLiteral do the same?

        Show
        julianhyde Julian Hyde added a comment - The code looks OK. Can you add a test case? For instance, does Drill's case treating a decimal literal as a double apply to Calcite? Can you review the javadoc related existing methods getOperandLiteralValue, isOperandNull. These methods treat casted literals as if they were literals. Should isOperandLiteral do the same?
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        1. Not really;
        Drill's default behavior (not correct though) is to treat Decimal Literal as Double [1]. When type-inference is taking place, Drill's inferencer needs to know if an operand is decimal literal.

        So it is more Drill specific issue.

        2. I see. I am enhancing it.

        [1] This is because of some ongoing work for decimal types in execution.

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - 1. Not really; Drill's default behavior (not correct though) is to treat Decimal Literal as Double [1] . When type-inference is taking place, Drill's inferencer needs to know if an operand is decimal literal. So it is more Drill specific issue. 2. I see. I am enhancing it. [1] This is because of some ongoing work for decimal types in execution.
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        Julian Hyde I updated the PR. Since these newly added methods are more like utilities which are not used in Calcite's core, should I still need to add test cases for them?

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - Julian Hyde I updated the PR. Since these newly added methods are more like utilities which are not used in Calcite's core, should I still need to add test cases for them?
        Hide
        julianhyde Julian Hyde added a comment -

        If they're not used by core, then they must be part of Calcite's public API. Do you think the public API should be tested?

        Show
        julianhyde Julian Hyde added a comment - If they're not used by core, then they must be part of Calcite's public API. Do you think the public API should be tested?
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        I added a few unit tests in UtilTest.java. Thanks for the review.

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - I added a few unit tests in UtilTest.java. Thanks for the review.
        Hide
        julianhyde Julian Hyde added a comment -

        Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/1b4209ec, with some fix-up in http://git-wip-us.apache.org/repos/asf/calcite/commit/44a6ba67 moving the tests to a new class. Thanks for the PR!

        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/1b4209ec , with some fix-up in http://git-wip-us.apache.org/repos/asf/calcite/commit/44a6ba67 moving the tests to a new class. Thanks for the PR!
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.8.0 (2016-06-13).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.8.0 (2016-06-13).

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            seanhychu Sean Hsuan-Yi Chu
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development