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

Support sub-queries (RexSubQuery) in RelToSqlConverter

    Details

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

      Description

      RelToSqlConverter does not currently support sub-queries (RexSubQuery), but it should.

      SqlToRelConverter, in the default configuration, converts sub-queries into Join clause. But when SqlToRelConverter.Config.expand = false, it does not convert them to Join. And that causes problems because RelToSqlConverter cannot handle them.

        Activity

        Hide
        ransom Zhiqiang He added a comment -
        Show
        ransom Zhiqiang He added a comment - https://github.com/apache/calcite/pull/477 please review it. thanks.
        Hide
        ransom Zhiqiang He added a comment -

        Julian Hyde can you help me to review it? thanks.

        Show
        ransom Zhiqiang He added a comment - Julian Hyde can you help me to review it? thanks.
        Hide
        julianhyde Julian Hyde added a comment -

        Wow, sorry I missed this the first time. I reviewed as of https://github.com/apache/calcite/pull/477/commits/347963bb5987137bea884c14668cf6aad58d45da.

        Looks great, but just one thing: in SqlImplementor.java line 616, what happens if rex is not an instance of RexSubQuery? Looks like there will be a ClassCastException as it falls through to the next case. Can you test that case, and fix the code? Or maybe rex instanceof RexSubQuery is always true in practice.

        Show
        julianhyde Julian Hyde added a comment - Wow, sorry I missed this the first time. I reviewed as of https://github.com/apache/calcite/pull/477/commits/347963bb5987137bea884c14668cf6aad58d45da . Looks great, but just one thing: in SqlImplementor.java line 616, what happens if rex is not an instance of RexSubQuery? Looks like there will be a ClassCastException as it falls through to the next case. Can you test that case, and fix the code? Or maybe rex instanceof RexSubQuery is always true in practice.
        Hide
        ransom Zhiqiang He added a comment - - edited

        Julian Hyde I'm already add instance for RexSubQuery and RexCall.
        but the rexnode of exists and scalar_query is always RexSubQuery . and the rexnode of NOT expression is always RexCall instance.
        so I can not test for the else branch. they will in default branch of switch-case expression.

        Show
        ransom Zhiqiang He added a comment - - edited Julian Hyde I'm already add instance for RexSubQuery and RexCall. but the rexnode of exists and scalar_query is always RexSubQuery . and the rexnode of NOT expression is always RexCall instance. so I can not test for the else branch. they will in default branch of switch-case expression.
        Hide
        julianhyde Julian Hyde added a comment -

        If you think that rex instanceof RexSubQuery is always true – or can't write a test where it's false – how about changing if (rex instanceof RexSubQuery) to assert rex instanceof RexSubQuery?

        I don't like code that is trying to handle an event that will never happen. By definition it is never tested.

        Show
        julianhyde Julian Hyde added a comment - If you think that rex instanceof RexSubQuery is always true – or can't write a test where it's false – how about changing if (rex instanceof RexSubQuery) to assert rex instanceof RexSubQuery ? I don't like code that is trying to handle an event that will never happen. By definition it is never tested.
        Hide
        ransom Zhiqiang He added a comment -

        Julian Hyde if expression is already changed to assert. please reivew it. thanks.

        Show
        ransom Zhiqiang He added a comment - Julian Hyde if expression is already changed to assert. please reivew it. thanks.
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/e43520a8 .
        Hide
        michaelmior Michael Mior added a comment -

        Resolved in release 1.14.0 (2017-10-01)

        Show
        michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)

          People

          • Assignee:
            ransom Zhiqiang He
            Reporter:
            ransom Zhiqiang He
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development