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

Support operator "%" as an alternative to "mod"

    Details

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

      Description

      Currently the following sql is not supported.

      SELECT a%3 FROM T

      We get the exception:

      Caused by: org.apache.calcite.sql.parser.SqlParseException: Lexical error at line 1, column 9. Encountered: "%" (37), after : ""
      at org.apache.calcite.sql.parser.impl.SqlParserImpl.convertException(SqlParserImpl.java:396)
      at org.apache.calcite.sql.parser.impl.SqlParserImpl.normalizeException(SqlParserImpl.java:129)

      In this JIRA. I'll fix this by support operator "%" as an alternative to "mod".
      Similar https://issues.apache.org/jira/browse/CALCITE-1374

        Issue Links

          Activity

          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)
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/61f1cf92 . Thanks for the PR, sunjincheng !
          Hide
          sunjincheng121 sunjincheng added a comment - - edited

          Ah, sorry I have not do the verify last update. Currently, I have passed the `mvn verify`and update the PR. . So please review it again! thanks!! Julian Hyde

          Show
          sunjincheng121 sunjincheng added a comment - - edited Ah, sorry I have not do the verify last update. Currently, I have passed the `mvn verify`and update the PR. . So please review it again! thanks!! Julian Hyde
          Hide
          julianhyde Julian Hyde added a comment -

          I reviewed https://github.com/apache/calcite/pull/506/commits/6303791766b3889c9a5f1dba1530b293a37fe614. Basically good, but there are still checkstyle exceptions and test failures so I can't commit.

          Show
          julianhyde Julian Hyde added a comment - I reviewed https://github.com/apache/calcite/pull/506/commits/6303791766b3889c9a5f1dba1530b293a37fe614 . Basically good, but there are still checkstyle exceptions and test failures so I can't commit.
          Hide
          sunjincheng121 sunjincheng added a comment -

          Hi, Julian HydeThanks for your review. and sorry for late reply. I had update the PR. according your comments. the changes as follows:

          1. Change the symbol `REMAINDER ` to `PERCENT_REMAINDER ` ;
          2. Mod function using `SqlKind.MOD`;
          3. Add ModPrecedenceTest;
          4. Add limitation of conformance levels.(MYQL_5 & LENIENT)
          5. Uses same code-generation code for both `%` and `mod function`.

          but i am not sure if the changes are 100% match your comments. I am grateful if can review the changes again.

          About your comments, i have a question: Why only allow % in the MYSQL and LENIENT. How can i know that before you told me. Is there some document you can share me to study ?

          changes reference:
          1. http://docs.oracle.com/cd/E11882_01/server.112/e41084/functions101.htm#SQLRF00668

          Show
          sunjincheng121 sunjincheng added a comment - Hi, Julian Hyde Thanks for your review. and sorry for late reply. I had update the PR. according your comments. the changes as follows: 1. Change the symbol `REMAINDER ` to `PERCENT_REMAINDER ` ; 2. Mod function using `SqlKind.MOD`; 3. Add ModPrecedenceTest; 4. Add limitation of conformance levels.(MYQL_5 & LENIENT) 5. Uses same code-generation code for both `%` and `mod function`. but i am not sure if the changes are 100% match your comments. I am grateful if can review the changes again. About your comments, i have a question: Why only allow % in the MYSQL and LENIENT. How can i know that before you told me. Is there some document you can share me to study ? changes reference: 1. http://docs.oracle.com/cd/E11882_01/server.112/e41084/functions101.htm#SQLRF00668
          Hide
          julianhyde Julian Hyde added a comment -

          Review comments:

          • I think you should call the parser symbol PERCENT. REMAINDER is a different function from MOD in Oracle, so using that name is confusing.
          • I'm glad you introduced SqlKind.MOD. But you should change SqlStdOperatorTable.MOD to use it.
          • Add tests that prove that the precedence of % is correct, e.g. a + b % c % d * e % f.
          • Only allow % in certain conformance levels (MYSQL and LENIENT)
          • Are you re-using the same code-generation code as MOD? Doesn't look like it. You should translate % to MOD as early as possible to, so you benefit from the existing infrastructure around MOD.
          Show
          julianhyde Julian Hyde added a comment - Review comments: I think you should call the parser symbol PERCENT. REMAINDER is a different function from MOD in Oracle, so using that name is confusing. I'm glad you introduced SqlKind.MOD. But you should change SqlStdOperatorTable.MOD to use it. Add tests that prove that the precedence of % is correct, e.g. a + b % c % d * e % f. Only allow % in certain conformance levels (MYSQL and LENIENT) Are you re-using the same code-generation code as MOD? Doesn't look like it. You should translate % to MOD as early as possible to, so you benefit from the existing infrastructure around MOD.
          Hide
          sunjincheng121 sunjincheng added a comment -

          Hi Julian Hyde, I have opened the PR. https://github.com/apache/calcite/pull/506

          In the PR. I have added `%` operator named `REMAINDER`, I'm not sure whether the name is make sense to you or not. I appreciated if you can review the PR.

          Best, Jincheng

          Show
          sunjincheng121 sunjincheng added a comment - Hi Julian Hyde , I have opened the PR. https://github.com/apache/calcite/pull/506 In the PR. I have added `%` operator named `REMAINDER`, I'm not sure whether the name is make sense to you or not. I appreciated if you can review the PR. Best, Jincheng

            People

            • Assignee:
              sunjincheng121 sunjincheng
              Reporter:
              sunjincheng121 sunjincheng
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development