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

Reducing NOT() should not collapse NOT(IS_TRUE) to IS_FALSE for nullable inputs

    Details

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

      Description

      In RexSimplify, when simplifying NOT(), we negate the input expression. But for IS_FALSE/IS_TRUE/IS_NOT_FALSE/IS_NOT_TRUE this cannot be just negated if the input is nullable.

      IS_FALSE(null) = false
      IS_TRUE(null) = false
      NOT(IS_FALSE(null)) = true != IS_TRUE(null)
      
      IS_NOT_FALSE(null) = true
      IS_NOT_TRUE(null) = true
      NOT(IS_NOT_FALSE(null)) = false != IS_NOT_TRUE(null)
      

        Activity

        Hide
        minjikim MinJi Kim added a comment -

        Here is my initial PR. Thanks!

        https://github.com/apache/calcite/pull/521

        Show
        minjikim MinJi Kim added a comment - Here is my initial PR. Thanks! https://github.com/apache/calcite/pull/521
        Hide
        julianhyde Julian Hyde added a comment - - edited

        Reviewing https://github.com/apache/calcite/pull/521/commits/90b34d7c28353ab7b819d62a85e38594591367fc:

        • I don't see the point of the change to ReduceExpressionsRule. The tests pass without it.
        • The out-of-order imports in SqlToRelConverterTest were missed due to a bug in checkstyle: https://github.com/checkstyle/checkstyle/issues/4981
        • Suppose we had IS_TRUE.negate() return IS_NOT_TRUE rather than IS_FALSE. Does this mean we can skip the special handling for null inputs?
        Show
        julianhyde Julian Hyde added a comment - - edited Reviewing https://github.com/apache/calcite/pull/521/commits/90b34d7c28353ab7b819d62a85e38594591367fc: I don't see the point of the change to ReduceExpressionsRule. The tests pass without it. The out-of-order imports in SqlToRelConverterTest were missed due to a bug in checkstyle: https://github.com/checkstyle/checkstyle/issues/4981 Suppose we had IS_TRUE.negate() return IS_NOT_TRUE rather than IS_FALSE. Does this mean we can skip the special handling for null inputs?
        Hide
        minjikim MinJi Kim added a comment -

        Julian Hyde I think your suggestion for IS_NOT_TRUE makes sense to me, and it would make things much cleaner. I will update the PR.

          true false null
        IS_TRUE true false false
        IS_NOT_TRUE false true true
        IS_FALSE false true false
        IS_NOT_FALSE true false true
        Show
        minjikim MinJi Kim added a comment - Julian Hyde I think your suggestion for IS_NOT_TRUE makes sense to me, and it would make things much cleaner. I will update the PR.   true false null IS_TRUE true false false IS_NOT_TRUE false true true IS_FALSE false true false IS_NOT_FALSE true false true
        Hide
        minjikim MinJi Kim added a comment - - edited

        Julian Hyde I made changes to the PR according to your comments. I modified the SqlKind.negate() and negateNullSafe() so that we negate the operators properly. Thanks again for reviewing!

        It looks like the change to ReduceExpressionsRule is no longer needed (fixed in CALCITE-1397, 0a1a190).

        https://github.com/apache/calcite/pull/521

        Show
        minjikim MinJi Kim added a comment - - edited Julian Hyde I made changes to the PR according to your comments. I modified the SqlKind.negate() and negateNullSafe() so that we negate the operators properly. Thanks again for reviewing! It looks like the change to ReduceExpressionsRule is no longer needed (fixed in CALCITE-1397 , 0a1a190). https://github.com/apache/calcite/pull/521
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/2156d826 .
        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:
            julianhyde Julian Hyde
            Reporter:
            minjikim MinJi Kim
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development