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

Return type of TIMESTAMP_ADD applied to a DATE should be TIMESTAMP if unit is smaller than DAY

    Details

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

      Description

      timestamp_add("MINUTE", 1, date '2016-06-15') returns 2016-06-15 since it returns a date and therefore truncates the minute informtion. timestamp_add should return timestamp instead of date for units less than date.

      timestamp_diff with date doesn't handle null properly (in type inference), converted type is INTEGER NOT NULL (when it should be null).

        Activity

        Show
        minjikim MinJi Kim added a comment - https://github.com/apache/calcite/pull/253
        Hide
        julianhyde Julian Hyde added a comment -
        • Better to add the test to SqlOperatorBaseTest than SqlValidatorTest. In fact you could just add a couple of lines to SqlValidatorTest.testTimestampDiff.
        • The tests you've written are a bit abstract. That increases the chance that there's a bug in the tests (or will be, after they've been "maintained"). I'd get rid of the for loops, write specific test cases, and don't worry that they're not 100% exhaustive.
        • Only put a strategy in ReturnTypes if it is re-usable by multiple operators.
        Show
        julianhyde Julian Hyde added a comment - Better to add the test to SqlOperatorBaseTest than SqlValidatorTest. In fact you could just add a couple of lines to SqlValidatorTest.testTimestampDiff. The tests you've written are a bit abstract. That increases the chance that there's a bug in the tests (or will be, after they've been "maintained"). I'd get rid of the for loops, write specific test cases, and don't worry that they're not 100% exhaustive. Only put a strategy in ReturnTypes if it is re-usable by multiple operators.
        Hide
        minjikim MinJi Kim added a comment - - edited

        Thanks for the review. I made the changes you suggested. Also, while adding the tests to SqlOperatorBaseTest, I noticed a few other problems (especially in timestampdiff with null dates). So, I also made those fixes in this patch. I also updated the jira. Thanks again!

        Show
        minjikim MinJi Kim added a comment - - edited Thanks for the review. I made the changes you suggested. Also, while adding the tests to SqlOperatorBaseTest, I noticed a few other problems (especially in timestampdiff with null dates). So, I also made those fixes in this patch. I also updated the jira. Thanks again!
        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/db6a3b3b . Thanks for the PR, MinJi Kim !
        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:
            minjikim MinJi Kim
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development