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

Add support for timestampadd / timestampdiff functions

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: next
    • Fix Version/s: 1.7.0
    • Component/s: core
    • Labels:
      None

      Description

      When calling timestampadd / timestampdiff with first parameter:

      (SQL_TSI_)MICROSECOND ((SQL_TSI_)FRAC_SECOND (deprecated but we can leave for older versions compatibility)), (SQL_TSI_)SECOND, (SQL_TSI_)MINUTE, (SQL_TSI_)HOUR, (SQL_TSI_)DAY, (SQL_TSI_)WEEK, (SQL_TSI_)MONTH, (SQL_TSI_)QUARTER, (SQL_TSI_)YEAR

      (ex: timestampadd(second, 1, current_datetime), calcite throws an error:

      Caused by: org.apache.calcite.sql.parser.SqlParseException: Encountered "( SECOND" at line 1, column 25.
      Was expecting one of:
      "(" "*" ...
      "(" ")" ...
      "(" "WITH" ...

        Issue Links

          Activity

          Hide
          arina Arina Ielchiieva added a comment -

          As part of solution I am registering all timestamp intervals as tokens to convert them to literals whenever they come inside timestamp[add|diff] structure.
          Also I have added them into SqlStdOperatorTable.

          Show
          arina Arina Ielchiieva added a comment - As part of solution I am registering all timestamp intervals as tokens to convert them to literals whenever they come inside timestamp [add|diff] structure. Also I have added them into SqlStdOperatorTable.
          Hide
          julianhyde Julian Hyde added a comment -

          You should probably do the same as we do for FLOOR. In particular note that StandardFloorCeilOptions() calls TimeUnit(). SqlLiteral.createSymbol may also be useful.

          Show
          julianhyde Julian Hyde added a comment - You should probably do the same as we do for FLOOR. In particular note that StandardFloorCeilOptions() calls TimeUnit(). SqlLiteral.createSymbol may also be useful.
          Hide
          julianhyde Julian Hyde added a comment -

          The PR looks good. I'd like to rework the PR so that the functions only appear when MySQL compatibility is enabled (see CALCITE-1066). And I'll write an implementation of the functions.

          Can you do one thing: Please add test cases to SqlOperatorBaseTest (based on, say, testDatePlusInterval) that call checkScalar, so that we know what results the functions should return given particular arguments.

          Show
          julianhyde Julian Hyde added a comment - The PR looks good. I'd like to rework the PR so that the functions only appear when MySQL compatibility is enabled (see CALCITE-1066 ). And I'll write an implementation of the functions. Can you do one thing: Please add test cases to SqlOperatorBaseTest (based on, say, testDatePlusInterval) that call checkScalar, so that we know what results the functions should return given particular arguments.
          Hide
          julianhyde Julian Hyde added a comment -

          Also, please add cases to SqlParserTest. This will ensure that we can unparse TIMESTAMPADD and TIMESTAMPDIFF correctly.

          Show
          julianhyde Julian Hyde added a comment - Also, please add cases to SqlParserTest. This will ensure that we can unparse TIMESTAMPADD and TIMESTAMPDIFF correctly.
          Hide
          arina Arina Ielchiieva added a comment -

          PR was updated.
          1. When writing lacking unit tests, I have realized that converting timestamp intervals to string, not a good idea, so I switched to SqlLiteral.createSymbol.
          2. Added unit test in SqlParserTest.
          3. Unit test in SqlOperatorBaseTest is not working, so currently it's ignored using boolean enable check.
          Test fails when RexToLixTranslator.translateCall

          RexImpTable.INSTANCE.get(operator);
          

          returns null which seems to be expected since timestampadd / diff are absent rexImpltable.map and in BuiltInMethod enum. But I am not sure if we need to update these classes.
          Could you please suggest what to do here?
          4. SqlOperatorBaseTest.testJdbc contains disabled tests for timestampadd / diff. Can we delete them, since our two functions are not parsed through SqlJdbcFunctionCall any more.

              if (false) {
                tester.checkScalar(
                    "{fn TIMESTAMPADD(interval, count, timestamp)}",
                    null,
                    "");
              }
              if (false) {
                tester.checkScalar(
                    "{fn TIMESTAMPDIFF(interval, timestamp1, timestamp2)}",
                    null,
                    "");
              }
          

          5. Not sure if we need to add our two functions in SqlJdbcFunction.JdbcToInternalLookupTable.map.

          Show
          arina Arina Ielchiieva added a comment - PR was updated. 1. When writing lacking unit tests, I have realized that converting timestamp intervals to string, not a good idea, so I switched to SqlLiteral.createSymbol. 2. Added unit test in SqlParserTest. 3. Unit test in SqlOperatorBaseTest is not working, so currently it's ignored using boolean enable check. Test fails when RexToLixTranslator.translateCall RexImpTable.INSTANCE.get( operator ); returns null which seems to be expected since timestampadd / diff are absent rexImpltable.map and in BuiltInMethod enum. But I am not sure if we need to update these classes. Could you please suggest what to do here? 4. SqlOperatorBaseTest.testJdbc contains disabled tests for timestampadd / diff. Can we delete them, since our two functions are not parsed through SqlJdbcFunctionCall any more. if ( false ) { tester.checkScalar( "{fn TIMESTAMPADD(interval, count, timestamp)}" , null , ""); } if ( false ) { tester.checkScalar( "{fn TIMESTAMPDIFF(interval, timestamp1, timestamp2)}" , null , ""); } 5. Not sure if we need to add our two functions in SqlJdbcFunction.JdbcToInternalLookupTable.map.
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/0b9ea986, with some further enhancements in http://git-wip-us.apache.org/repos/asf/calcite/commit/4ac82a30 to implement them in Enumerable convention and expose them via JDBC function syntax. Arina Ielchiieva, Thanks for the PR!

          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/0b9ea986 , with some further enhancements in http://git-wip-us.apache.org/repos/asf/calcite/commit/4ac82a30 to implement them in Enumerable convention and expose them via JDBC function syntax. Arina Ielchiieva , Thanks for the PR!
          Hide
          arina Arina Ielchiieva added a comment -

          SqlOperatorBaseTest.testTimestampDiff has unexpected results for timestampdiff function.
          For example, when you calculate timestampdiff(interval, smaller value, greater value) expected result is positive difference, if timestampdiff(interval, greater value, smaller value) - negative.

          Queries I have executed in MySql:

          select timestampdiff(HOUR, str_to_date('2016-02-24 12:42:25.000', '%Y-%m-%d %H:%i:%s.%f'), str_to_date('2016-02-24 15:42:25.000', '%Y-%m-%d %H:%i:%s.%f')) as ACTUAL;
          -- 3
          select timestampdiff(MICROSECOND, str_to_date('2016-02-24 12:42:25.000', '%Y-%m-%d %H:%i:%s.%f'), str_to_date('2016-02-24 12:42:20.000', '%Y-%m-%d %H:%i:%s.%f')) as ACTUAL;
          -- -5000000
          
          Show
          arina Arina Ielchiieva added a comment - SqlOperatorBaseTest.testTimestampDiff has unexpected results for timestampdiff function. For example, when you calculate timestampdiff(interval, smaller value, greater value) expected result is positive difference, if timestampdiff(interval, greater value, smaller value) - negative. Queries I have executed in MySql: select timestampdiff(HOUR, str_to_date('2016-02-24 12:42:25.000', '%Y-%m-%d %H:%i:%s.%f'), str_to_date('2016-02-24 15:42:25.000', '%Y-%m-%d %H:%i:%s.%f')) as ACTUAL; -- 3 select timestampdiff(MICROSECOND, str_to_date('2016-02-24 12:42:25.000', '%Y-%m-%d %H:%i:%s.%f'), str_to_date('2016-02-24 12:42:20.000', '%Y-%m-%d %H:%i:%s.%f')) as ACTUAL; -- -5000000
          Hide
          julianhyde Julian Hyde added a comment -

          I'm working on a fix for this.

          Show
          julianhyde Julian Hyde added a comment - I'm working on a fix for this.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/b8d271b9 .
          Show
          julianhyde Julian Hyde added a comment - Amended fix in http://git-wip-us.apache.org/repos/asf/calcite/commit/ccb48675 .
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.7.0 (2016-03-22).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.7.0 (2016-03-22).

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              arina Arina Ielchiieva
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development