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

Simplifying CAST(characterLiteral as TIMESTAMP) should round the sub-second fraction

    Details

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

      Description

      Timestamps are losing sub-second parts after Calcite literal constant reduction.

      select cast("1970-12-31 15:59:58.174" as TIMESTAMP) from src limit 1;
      

      yields 1970-12-31 15:59:58 if CBO is enabled in Hive. ZonelessTimestamp.toString() contains a truncation 0-19 (ie. removes sub-second part from string representation).

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.12.0 (2017-03-24).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).
          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/bb6ae0ac . Thanks for the PR, Remus Rusanu !
          Hide
          rusanu Remus Rusanu added a comment - - edited

          Seems you figured out how to run Quidem tests. How could the documentation be improved? Should we add more documentation to the QuidemTest class?

          In the end the page at https://calcite.apache.org/docs/howto.html#running-integration-tests was the one that got me on the right track. Via trial-and-error I ended up modifying misc.iq and running the following, because it was quick and yielded the result and diff I needed:

          >mvn -f core/pom.xml verify -Pti -Dtest=Quidem
          

          although I'm not sure the -Dtest=... does what I think it does, because when the .iq test begin to pass then many more tests run, so I believe I was just lucky in the order. Also, the actual .iq expected result and plan I copy-pasted them from the mvn diff output, I did not locate any output file in core/target to copy from.

          As for what to improve, I think the calcite.apache.org HOWTO is the best resource to get the ball rolling. Had the QuidemTest class contain detailed explanation exactly step by step what to do, I would still probably not stumble upon it, and not know where to start.

          Show
          rusanu Remus Rusanu added a comment - - edited Seems you figured out how to run Quidem tests. How could the documentation be improved? Should we add more documentation to the QuidemTest class? In the end the page at https://calcite.apache.org/docs/howto.html#running-integration-tests was the one that got me on the right track. Via trial-and-error I ended up modifying misc.iq and running the following, because it was quick and yielded the result and diff I needed: >mvn -f core/pom.xml verify -Pti -Dtest=Quidem although I'm not sure the -Dtest=... does what I think it does, because when the .iq test begin to pass then many more tests run, so I believe I was just lucky in the order. Also, the actual .iq expected result and plan I copy-pasted them from the mvn diff output, I did not locate any output file in core/target to copy from. As for what to improve, I think the calcite.apache.org HOWTO is the best resource to get the ball rolling. Had the QuidemTest class contain detailed explanation exactly step by step what to do, I would still probably not stumble upon it, and not know where to start.
          Hide
          rusanu Remus Rusanu added a comment -

          When you commit, please also push new snapshot to Maven. Thanks!

          Show
          rusanu Remus Rusanu added a comment - When you commit, please also push new snapshot to Maven. Thanks!
          Hide
          julianhyde Julian Hyde added a comment -

          Reviewing the code now. It seems to make sense to call roundTimestampToPrecision from makeLiteral, so that it gets hit by all code paths. Will commit shortly.

          Show
          julianhyde Julian Hyde added a comment - Reviewing the code now. It seems to make sense to call roundTimestampToPrecision from makeLiteral , so that it gets hit by all code paths. Will commit shortly.
          Hide
          julianhyde Julian Hyde added a comment -

          Anywhere I can get quickly up to speed with .iq maven integration in Calcite?

          Seems you figured out how to run Quidem tests. How could the documentation be improved? Should we add more documentation to the QuidemTest class?

          Show
          julianhyde Julian Hyde added a comment - Anywhere I can get quickly up to speed with .iq maven integration in Calcite? Seems you figured out how to run Quidem tests. How could the documentation be improved? Should we add more documentation to the QuidemTest class?
          Hide
          rusanu Remus Rusanu added a comment -

          I added test case.

          I also added the same logic to makeTimestampLiteral and made the ZonelessTimestamp digest display the sub-second fraction, if relevant. In the process of writing tests I also found some issues and opened CALCITE-1664, CALCITE-1663 and CALCITE-1662.

          Show
          rusanu Remus Rusanu added a comment - I added test case. I also added the same logic to makeTimestampLiteral and made the ZonelessTimestamp digest display the sub-second fraction, if relevant. In the process of writing tests I also found some issues and opened CALCITE-1664 , CALCITE-1663 and CALCITE-1662 .
          Hide
          rusanu Remus Rusanu added a comment -

          Anywhere I can get quickly up to speed with .iq maven integration in Calcite? I did a quick read of https://github.com/julianhyde/quidem and modified core/src/test/resources/sql/misc.iq with my test(s), but running mvn test produces no new .iq files on core/target. I don't think it even run my new tests, mvn test succeeds, despite me not adding correct expected output.

          Show
          rusanu Remus Rusanu added a comment - Anywhere I can get quickly up to speed with .iq maven integration in Calcite? I did a quick read of https://github.com/julianhyde/quidem and modified core/src/test/resources/sql/misc.iq with my test(s), but running mvn test produces no new .iq files on core/target. I don't think it even run my new tests, mvn test succeeds, despite me not adding correct expected output.
          Hide
          julianhyde Julian Hyde added a comment -

          I need a test case. Also you have mIs-spelled "precision".

          Show
          julianhyde Julian Hyde added a comment - I need a test case. Also you have mIs-spelled "precision".
          Show
          rusanu Remus Rusanu added a comment - https://github.com/apache/calcite/pull/384
          Hide
          rusanu Remus Rusanu added a comment - - edited

          Looks like there is a disagreement here between timestmap scale vs. precision. In RexBuilder.makeCast the type scale is inspected. However, all default TIMESTMAP type system uses the precision (default 9) and does not specify the scale, as far as I can tell. I also looked at the standard and I did not find mention of scale vis-a-vis timestamps, only precision.

          Show
          rusanu Remus Rusanu added a comment - - edited Looks like there is a disagreement here between timestmap scale vs. precision . In RexBuilder.makeCast the type scale is inspected. However, all default TIMESTMAP type system uses the precision (default 9) and does not specify the scale, as far as I can tell. I also looked at the standard and I did not find mention of scale vis-a-vis timestamps, only precision.

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              rusanu Remus Rusanu
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development