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

Interval fractional second precision returns wrong value

    Details

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

      Description

      Unless I'm wrong, I believe SqlIntervalQualifer#getFractionalSecondPrecisionPreservingDefault() returns the wrong field:

        public int getFractionalSecondPrecision(RelDataTypeSystem typeSystem) {
          if (fractionalSecondPrecision == RelDataType.PRECISION_NOT_SPECIFIED) {
            return typeName().getDefaultScale();
          } else {
            return fractionalSecondPrecision;
          }
        }
      
        public int getFractionalSecondPrecisionPreservingDefault() {
          if (useDefaultFractionalSecondPrecision()) {
            return RelDataType.PRECISION_NOT_SPECIFIED;
          } else {
            return startPrecision;
          }
        }
      

        Activity

        Hide
        laurentgo Laurent Goujon added a comment -
        Show
        laurentgo Laurent Goujon added a comment - Proposed change: https://github.com/apache/calcite/pull/309
        Hide
        julianhyde Julian Hyde added a comment -

        I'm not sure. I tinkered with that code a while ago, and came to the conclusion that while it looks wrong, it's actually correct. Can you provide a test case that proves there's a bug here?

        Show
        julianhyde Julian Hyde added a comment - I'm not sure. I tinkered with that code a while ago, and came to the conclusion that while it looks wrong, it's actually correct. Can you provide a test case that proves there's a bug here?
        Hide
        laurentgo Laurent Goujon added a comment -

        The issue surfaced when testing a change in Drill in the view system, where the schema is serialized/deserialized and during deserialization the types are not matching anymore:
        https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java#L185

        Maybe this method is doing the right thing, but why then:

        • the name of the method starts with getFractionalSecondPrecision?
        • getFractionalSecondPrecision(RelDataTypeSystem) returns fractionalSecondPrecision, and not startPrecision
        • what's the difference then with getStartPrecisionPreservingDefault()?

        The two places where I saw this method used is in Drill, and in static method combineFractionalSecondPrecisionPreservingDefault, where this method is used to combine IntervalSqlType instances, by storing the result into the field fracPrec.

        So from the look of it, everything seems to believe this method is supposed to return the fractional second precision.

        I'm willing to add a test case, but I'm not sure which kind (also calcite didn't fail with my patch so I guess it means there's no existing test depending on that behaviour)?

        Show
        laurentgo Laurent Goujon added a comment - The issue surfaced when testing a change in Drill in the view system, where the schema is serialized/deserialized and during deserialization the types are not matching anymore: https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java#L185 Maybe this method is doing the right thing, but why then: the name of the method starts with getFractionalSecondPrecision ? getFractionalSecondPrecision(RelDataTypeSystem) returns fractionalSecondPrecision , and not startPrecision what's the difference then with getStartPrecisionPreservingDefault() ? The two places where I saw this method used is in Drill, and in static method combineFractionalSecondPrecisionPreservingDefault , where this method is used to combine IntervalSqlType instances, by storing the result into the field fracPrec. So from the look of it, everything seems to believe this method is supposed to return the fractional second precision. I'm willing to add a test case, but I'm not sure which kind (also calcite didn't fail with my patch so I guess it means there's no existing test depending on that behaviour)?
        Hide
        julianhyde Julian Hyde added a comment -

        I'm not surprised at all that there's no existing test. Having spent the day studying this code, you probably know more about it than anyone else in the world, so you are in a good position to write a few tests.

        Show
        julianhyde Julian Hyde added a comment - I'm not surprised at all that there's no existing test. Having spent the day studying this code, you probably know more about it than anyone else in the world, so you are in a good position to write a few tests.
        Hide
        laurentgo Laurent Goujon added a comment - - edited

        if you mean "debugging what the value was not the one I was expecting in Drill, and clicking on call hierarchy on my IDE to see where this function was used", yes I deeply studied the code and the code is present from the early days I guess (if I'm correct, introduced in commit b0dab683059fa1163dc95cb9ea7540ad6a4968ab)

        Show
        laurentgo Laurent Goujon added a comment - - edited if you mean "debugging what the value was not the one I was expecting in Drill, and clicking on call hierarchy on my IDE to see where this function was used", yes I deeply studied the code and the code is present from the early days I guess (if I'm correct, introduced in commit b0dab683059fa1163dc95cb9ea7540ad6a4968ab)
        Hide
        laurentgo Laurent Goujon added a comment -

        Updated my pull request with a new test case in SqlValidatorTest:

            checkExpType(
                "CASE 1 WHEN 1 THEN INTERVAL '12 3:4:5.6' DAY TO SECOND(6) WHEN 2 THEN INTERVAL '12 3:4:5.6' DAY TO SECOND(9) END",
                "INTERVAL DAY TO SECOND(9)");
        

        With the current code, returned type is INTERVAL DAY TO SECOND(6).

        Show
        laurentgo Laurent Goujon added a comment - Updated my pull request with a new test case in SqlValidatorTest: checkExpType( "CASE 1 WHEN 1 THEN INTERVAL '12 3:4:5.6' DAY TO SECOND(6) WHEN 2 THEN INTERVAL '12 3:4:5.6' DAY TO SECOND(9) END" , "INTERVAL DAY TO SECOND(9)" ); With the current code, returned type is INTERVAL DAY TO SECOND(6) .
        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/25a7d938 ; thanks for the PR, Laurent Goujon !
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.11.0 (2017-01-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            laurentgo Laurent Goujon
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development