Details

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

      Description

      The precedence of the NOT operator is currently too high; it should be lower than '='. WHERE NOT deptno = 10 works on other databases.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Here's a test case: https://github.com/julianhyde/calcite/tree/1095-not-precedence
          Hide
          mahongbin hongbin ma added a comment -

          hi Julian Hyde is there a fix plan for this bug?

          Show
          mahongbin hongbin ma added a comment - hi Julian Hyde is there a fix plan for this bug?
          Hide
          julianhyde Julian Hyde added a comment -

          I have a fix in https://github.com/julianhyde/calcite/tree/1095-not-precedence. Can someone please review? It required significant changes to the parsing process.

          Show
          julianhyde Julian Hyde added a comment - I have a fix in https://github.com/julianhyde/calcite/tree/1095-not-precedence . Can someone please review? It required significant changes to the parsing process.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, I took a quick look and patch LGTM.

          I just have a thought around hardcoding the precedence, maybe for a follow-up if you think it makes sense. We could store the functions in a static data structure so the precedence is inferred automatically depending on their position there. We would be able to see clearly the precedence just by looking at this structure, and we would not need to use magical numbers (24, 26, 28, etc) in the function constructor, which I guess might get a bit tricky in case new operators are added e.g. either in Calcite itself or by systems that use it as a library.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I took a quick look and patch LGTM. I just have a thought around hardcoding the precedence, maybe for a follow-up if you think it makes sense. We could store the functions in a static data structure so the precedence is inferred automatically depending on their position there. We would be able to see clearly the precedence just by looking at this structure, and we would not need to use magical numbers ( 24, 26, 28, etc ) in the function constructor, which I guess might get a bit tricky in case new operators are added e.g. either in Calcite itself or by systems that use it as a library.
          Hide
          julianhyde Julian Hyde added a comment -

          I agree that the magic numbers suck. As a user you are only really interested in the relative order, e.g. does = have lower, higher or the same precedence as LIKE.

          However, we need to let people write user-defined operators, and a precedence number seems to be the most convenient way to slot them into the right place in the order.

          I think we should have a test that sorts all operators based on precedence and checks that they come out in the order that we expect, per https://github.com/julianhyde/calcite/blob/1095-not-precedence/site/_docs/reference.md#operator-precedence.

          Show
          julianhyde Julian Hyde added a comment - I agree that the magic numbers suck. As a user you are only really interested in the relative order, e.g. does = have lower, higher or the same precedence as LIKE . However, we need to let people write user-defined operators, and a precedence number seems to be the most convenient way to slot them into the right place in the order. I think we should have a test that sorts all operators based on precedence and checks that they come out in the order that we expect, per https://github.com/julianhyde/calcite/blob/1095-not-precedence/site/_docs/reference.md#operator-precedence .
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Yes, that's a nice idea! The result of such test could be added to documentation too.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Yes, that's a nice idea! The result of such test could be added to documentation too.
          Hide
          maryannxue Maryann Xue added a comment -

          Tested the branch against Calcite-Phoenix and the result was good.

          Show
          maryannxue Maryann Xue added a comment - Tested the branch against Calcite-Phoenix and the result was good.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

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

          Please, leave a comment if there are any issues left.

          I have created CALCITE-1376 to follow-up with the test.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/339ac13 . Please, leave a comment if there are any issues left. I have created CALCITE-1376 to follow-up with the test.
          Hide
          julianhyde Julian Hyde added a comment -

          Sorry I didn't get chance to mark this (and a few other JIRA cases) fixed when I pushed my commits. (It was at the end of my work day; was going to do it this morning.)

          My commit does actually include the test, so I'll close CALCITE-1376.

          Show
          julianhyde Julian Hyde added a comment - Sorry I didn't get chance to mark this (and a few other JIRA cases) fixed when I pushed my commits. (It was at the end of my work day; was going to do it this morning.) My commit does actually include the test, so I'll close CALCITE-1376 .
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.9.0 (2016-09-22)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              julianhyde Julian Hyde
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development