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

Check year, month, day, hour, minute and second ranges for date and time literals

    Details

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

      Description

      Currently, if the year that is passed into DateString constructor has five digits, the first digit is trimmed. This trimming happens in the DateTimeStringUtils.int4() method:

        private static void int4(StringBuilder buf, int i) {
          buf.append((char) ('0' + (i / 1000) % 10));
          buf.append((char) ('0' + (i / 100) % 10));
          buf.append((char) ('0' + (i / 10) % 10));
          buf.append((char) ('0' + i % 10));
        }
      

      The same problem with month and day values.

      Instead of trimming the value, the correct behaviour is to throw an exception if any of the values are outside the expected range.

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.15.0 (2017-12-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
        Hide
        julianhyde Julian Hyde added a comment -

        Fixed in 237b6277. Thanks for the PR, Volodymyr Vysotskyi!

        (I moved your pre-condition checks into the DateString and TimeString constructors, because invariants are good. Hope you don't mind.)

        Show
        julianhyde Julian Hyde added a comment - Fixed in 237b6277 . Thanks for the PR, Volodymyr Vysotskyi ! (I moved your pre-condition checks into the DateString and TimeString constructors, because invariants are good. Hope you don't mind.)
        Hide
        vvysotskyi Volodymyr Vysotskyi added a comment -

        Julian Hyde, thanks for the review, I have changed commit message, Jira and pull request titles; replaced AssertionError by IllegalArgumentException; modified unit tests to call fail() instead of throwing the exception from the catch block. Could you please take a look again?

        Show
        vvysotskyi Volodymyr Vysotskyi added a comment - Julian Hyde , thanks for the review, I have changed commit message, Jira and pull request titles; replaced AssertionError by IllegalArgumentException ; modified unit tests to call fail() instead of throwing the exception from the catch block. Could you please take a look again?
        Hide
        julianhyde Julian Hyde added a comment -

        Reviewing https://github.com/apache/calcite/pull/567/commits/4a49ec9f9fe56a5ff1882ebdf446a6355aa0a232:

        • I see you decided that int4 is not to blame - good - low-level routines should not be responsible for checking user input.
        • Can you re-title this case and the commit
        • Throw IllegalArgumentException not AssertionError - this is an API error not an internal error
        • We usually do exception testing a bit differently. The tests should call fail() if there is no exception, not throw anything if the expected exception is thrown
        Show
        julianhyde Julian Hyde added a comment - Reviewing https://github.com/apache/calcite/pull/567/commits/4a49ec9f9fe56a5ff1882ebdf446a6355aa0a232: I see you decided that int4 is not to blame - good - low-level routines should not be responsible for checking user input. Can you re-title this case and the commit Throw IllegalArgumentException not AssertionError - this is an API error not an internal error We usually do exception testing a bit differently. The tests should call fail() if there is no exception, not throw anything if the expected exception is thrown
        Hide
        vvysotskyi Volodymyr Vysotskyi added a comment -

        I have created pull request for this issue: https://github.com/apache/calcite/pull/567.

        Show
        vvysotskyi Volodymyr Vysotskyi added a comment - I have created pull request for this issue: https://github.com/apache/calcite/pull/567 .

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            vvysotskyi Volodymyr Vysotskyi
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development