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

Support ANY type with binary compare / arithmetic operators

    Details

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

      Description

      Currently Calcite doesn't support applying binary compare / arithmetic operators with ANY type. One of example is CollectionTypeTest.testAccessNestedMapWithAnyTypeWithoutCast(). Without explicit casting, it can't find the matching backup method, and complaining there's no SqlFunctions.eq(Object, int).

      There seems to several ways to resolve this, but at least we don't want to make operator backup method for every combination of types. Needs to avoid this approach.

      When we're addressing this by having backup method, since we don't know the runtime type for ANY type, even if we succeed to call backup method with (Object, Object) parameters, two types can be different. This is OK for other types, but not Number types. This should be well cared, too.

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        MinJi Kim, Jinfeng Ni, You've both done work on ANY. Any ideas?

        Show
        julianhyde Julian Hyde added a comment - MinJi Kim , Jinfeng Ni , You've both done work on ANY. Any ideas?
        Hide
        kabhwan Jungtaek Lim added a comment -

        I've submitted a patch: https://github.com/apache/calcite/pull/311

        Since type information is only available in runtime, I just try to convert all cases to match (Object, Object), and let backup methods care the parameters nicely.
        During handling parameters which are Number type, I just converted the parameter to BigDecimal so that we can simplify overloading methods, but since there're downsides of the approach I'm OK to find other ways if we think it's not acceptable.

        Show
        kabhwan Jungtaek Lim added a comment - I've submitted a patch: https://github.com/apache/calcite/pull/311 Since type information is only available in runtime, I just try to convert all cases to match (Object, Object), and let backup methods care the parameters nicely. During handling parameters which are Number type, I just converted the parameter to BigDecimal so that we can simplify overloading methods, but since there're downsides of the approach I'm OK to find other ways if we think it's not acceptable.
        Hide
        minjikim MinJi Kim added a comment - - edited

        Julian Hyde and Jungtaek Lim I will take a look at the patch.

        Show
        minjikim MinJi Kim added a comment - - edited Julian Hyde and Jungtaek Lim I will take a look at the patch.
        Hide
        kabhwan Jungtaek Lim added a comment -

        MinJi Kim Just a reminder: Could you have a look?

        Show
        kabhwan Jungtaek Lim added a comment - MinJi Kim Just a reminder: Could you have a look?
        Hide
        julianhyde Julian Hyde added a comment -

        The new `SqlFunctions` methods support a lot of permutations, so I think we need corresponding tests added to `SqlFunctionsTest`.

        I am worried that bugs in the code generator (CALCITE-1467 is a recent example) will end up calling a function with Object arguments and giving poor runtime performance. How can we prevent that? Could we change the name of these fallback functions, say multiply(Object, Object) becomes multiplyAny(Object, Object)?

        Show
        julianhyde Julian Hyde added a comment - The new `SqlFunctions` methods support a lot of permutations, so I think we need corresponding tests added to `SqlFunctionsTest`. I am worried that bugs in the code generator ( CALCITE-1467 is a recent example) will end up calling a function with Object arguments and giving poor runtime performance. How can we prevent that? Could we change the name of these fallback functions, say multiply(Object, Object) becomes multiplyAny(Object, Object) ?
        Hide
        kabhwan Jungtaek Lim added a comment -

        Yes agreed. Let me add some tests on SqlFunctionsTest.

        And I also think we can append postfix "Any" to backupMethod and call it as you suggested. ANY type with not yet supported backupMethod will fail, but that seems more clear on avoiding false-positive.

        One thing to make it clear: Do we have any chance to receive Object type while it's not rel type of ANY?

        Show
        kabhwan Jungtaek Lim added a comment - Yes agreed. Let me add some tests on SqlFunctionsTest. And I also think we can append postfix "Any" to backupMethod and call it as you suggested. ANY type with not yet supported backupMethod will fail, but that seems more clear on avoiding false-positive. One thing to make it clear: Do we have any chance to receive Object type while it's not rel type of ANY?
        Hide
        julianhyde Julian Hyde added a comment -

        One thing to make it clear: Do we have any chance to receive Object type while it's not rel type of ANY?

        If there were no code generation bugs, probably not. But there are (and likely always will be) code generation bugs.

        Show
        julianhyde Julian Hyde added a comment - One thing to make it clear: Do we have any chance to receive Object type while it's not rel type of ANY? If there were no code generation bugs, probably not. But there are (and likely always will be) code generation bugs.
        Hide
        kabhwan Jungtaek Lim added a comment -

        Just found out we have RexCall, so we can check actual Rel type, instead of relying on Java type on Expression. That may be not issue at all.
        (I'm referring BinaryImplementor.implement())

        Show
        kabhwan Jungtaek Lim added a comment - Just found out we have RexCall, so we can check actual Rel type, instead of relying on Java type on Expression. That may be not issue at all. (I'm referring BinaryImplementor.implement())
        Hide
        kabhwan Jungtaek Lim added a comment -

        Julian Hyde
        Addressed your comments (both pull request and issue).
        Could you take a look again? Thanks in advance!

        Show
        kabhwan Jungtaek Lim added a comment - Julian Hyde Addressed your comments (both pull request and issue). Could you take a look again? Thanks in advance!
        Hide
        julianhyde Julian Hyde added a comment -

        Looks good. I will commit after the avatica-1.9.0 vote has passed.

        Show
        julianhyde Julian Hyde added a comment - Looks good. I will commit after the avatica-1.9.0 vote has passed.
        Hide
        kabhwan Jungtaek Lim added a comment -

        Thanks for reviewing and fixing up!

        Show
        kabhwan Jungtaek Lim added a comment - Thanks for reviewing and fixing up!
        Show
        julianhyde Julian Hyde added a comment - - edited Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/80240720 , with a small fix-up in http://git-wip-us.apache.org/repos/asf/calcite/commit/01d4760c . Thanks for the PR, Jungtaek Lim !
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.11.0 (2017-01-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

          People

          • Assignee:
            kabhwan Jungtaek Lim
            Reporter:
            kabhwan Jungtaek Lim
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development