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

Fix return type of item operator when operand type is not known

    Details

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

      Description

      While inferring the return type of the item operator (SqlItemOperator. inferReturnType()) when the SqlTypeName of the operand is ANY we set the nullability of return type to be false. Since the type of the operand is not known at this point we cannot know the return type and its nullability.

      Setting nullability to false was causing wrong results in Drill when we were performing count(item op), Calcite would set nullability to be false and the count(item op) would get replaced with count(0) causing wrong results if the column is eventually resolved to be nullable and has nulls in it.

        Activity

        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        Can you please provide SQL and the operator in question that causes the problem?

        count(0) causing wrong results if the column is eventually resolved to be nullable and has nulls in it.

        select count(u) from (select null as u...) should return 0;
        select count(u) from (select null as u... where 1=2) should return 0 as well;

        In SQL, count is somewhat special since it is always non-nullable.

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Can you please provide SQL and the operator in question that causes the problem? count(0) causing wrong results if the column is eventually resolved to be nullable and has nulls in it. select count(u) from (select null as u...) should return 0; select count(u) from (select null as u... where 1=2) should return 0 as well; In SQL, count is somewhat special since it is always non-nullable.
        Hide
        mehant Mehant Baid added a comment -

        Yes count is always non-nullable, however count of a nullable column versus a non-nullable column is the issue here.
        Consider following query
        select count(t.col1['col2']) from foo as t;

        Calcite transforms this query to
        select count(0) from foo as t

        This causes wrong results since we end up including the nulls in the count as well. The reason this transformation happens is because the nullability of the item operator is set to false.

        Show
        mehant Mehant Baid added a comment - Yes count is always non-nullable, however count of a nullable column versus a non-nullable column is the issue here. Consider following query select count(t.col1 ['col2'] ) from foo as t; Calcite transforms this query to select count(0) from foo as t This causes wrong results since we end up including the nulls in the count as well. The reason this transformation happens is because the nullability of the item operator is set to false.
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        Well, org.apache.calcite.sql.fun.SqlItemOperator#inferReturnType seems to really miss createTypeWithNullability(..., true) for ANY first operand

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Well, org.apache.calcite.sql.fun.SqlItemOperator#inferReturnType seems to really miss createTypeWithNullability(..., true) for ANY first operand
        Hide
        mehant Mehant Baid added a comment -

        Yes, that is essentially the patch I've posted, please review.

        Show
        mehant Mehant Baid added a comment - Yes, that is essentially the patch I've posted, please review.
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        1) Can you please add test for this case?
        2) Can you please move typeFactory to the next line so typeFactory and createSqlType are on the same line?

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - 1) Can you please add test for this case? 2) Can you please move typeFactory to the next line so typeFactory and createSqlType are on the same line?
        Hide
        mehant Mehant Baid added a comment -

        Can you please add test for this case?

        • For testing I am planning to add a query which looks like: "select t.a.b from foo" and check the return type of the expression (t.a.b) for nullability. However could you please point me to an example where I can use ITEM operators whose SqlTypeName is ANY. Most of the examples I found were using values(1) as the data source.

        Can you please move typeFactory to the next line so typeFactory and createSqlType are on the same line?

        • Done.
        Show
        mehant Mehant Baid added a comment - Can you please add test for this case? For testing I am planning to add a query which looks like: "select t.a.b from foo" and check the return type of the expression (t.a.b) for nullability. However could you please point me to an example where I can use ITEM operators whose SqlTypeName is ANY. Most of the examples I found were using values(1) as the data source. Can you please move typeFactory to the next line so typeFactory and createSqlType are on the same line? Done.
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        However could you please point me to an example where I can use ITEM operators whose SqlTypeName is ANY. Most of the examples I found were using values(1) as the data source.

        I think it would be relevant to update SqlOperatorBaseTest.testItemOp.
        Does select cast(null as any)['x'] from values(1) do the trick?

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - However could you please point me to an example where I can use ITEM operators whose SqlTypeName is ANY. Most of the examples I found were using values(1) as the data source. I think it would be relevant to update SqlOperatorBaseTest.testItemOp . Does select cast(null as any)['x'] from values(1) do the trick?
        Hide
        mehant Mehant Baid added a comment -

        Yes, that did the trick. I've updated the patch based on your suggestions. Please review.

        updated pull request: https://github.com/apache/incubator-calcite/pull/37/commits

        Show
        mehant Mehant Baid added a comment - Yes, that did the trick. I've updated the patch based on your suggestions. Please review. updated pull request: https://github.com/apache/incubator-calcite/pull/37/commits
        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Merged in https://git-wip-us.apache.org/repos/asf?p=incubator-calcite.git;a=commit;h=8cdc6634267e138e237bba785e46ea0017c2a0fd Thanks for the fix.
        Hide
        julianhyde Julian Hyde added a comment -

        Closing now that 1.0.0-incubating has been released.

        Show
        julianhyde Julian Hyde added a comment - Closing now that 1.0.0-incubating has been released.

          People

          • Assignee:
            mehant Mehant Baid
            Reporter:
            mehant Mehant Baid
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development