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

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Hide
            Permalink
            julianhyde Julian Hyde added a comment -

            Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/2156d826.

            Show
            julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/2156d826 .
            Hide
            Permalink
            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