Fix rounding in cast from TIME to TIMESTAMP #5742
Conversation
There was a problem hiding this comment.
Couple comments, otherwise LGTM.
Per #5742 (comment), i think we should not close #5738 when merging this.
presto-main/src/main/java/io/prestosql/operator/scalar/time/TimeToTimestampCast.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/type/SqlTimestamp.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/type/SqlTimestamp.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/type/SqlTimestamp.java
Outdated
Show resolved
Hide resolved
| @@ -79,10 +79,6 @@ public static long round(long value, int magnitude) | |||
| */ | |||
| static long rescale(long value, int fromPrecision, int toPrecision) | |||
| { | |||
| if (value < 0) { | |||
| throw new IllegalArgumentException("value must be >= 0"); | |||
There was a problem hiding this comment.
rescale method works great when the rounding is known to be not necessary.
For cases when rounding may be necessary, round, floorDiv seem to offer well defined semantics.
In this commit you're using the rounding-or-truncation of this method, but i'd change it to expect that value is pre-rounded. (actual check may be too expensive for runtime, idk).
This is more relevant when we allow negative numbers here.
There was a problem hiding this comment.
rescale method works great when the rounding is known to be not necessary.
Correct, and that's the exact purpose of this method. The only reason we had this check is to protect against trying to rescale epoch values which can be negative. But it doesn't matter. I'll get rid of the call to rescale as you suggested above and revert this change.
martint
force-pushed
the
time-to-timestamp
branch
from
656af44 to
3b3be4d
Compare
presto-spi/src/test/java/io/prestosql/spi/type/TestTimestamps.java
Outdated
Show resolved
Hide resolved
presto-spi/src/test/java/io/prestosql/spi/type/TestTimestamps.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/testing/DateTimeTestingUtils.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/type/SqlTimestampWithTimeZone.java
Show resolved
Hide resolved
martint
force-pushed
the
time-to-timestamp
branch
from
3b3be4d to
43bd153
Compare
This is to distinguish between the semantics of newInstance, which is not supposed to do any conversions (rounding, etc), with the version that constructs a instance from an Instance by applying any necessary rounding.
For negative numbers, it will produce a result different from the original value, which is incorrect.
Previously, it was applying rounding rules, which ended up masking potential problems in operations that are expected to do proper rounding.
They were in a separate class for when we had to parameterize the tests for legacy vs new timestamp semantics. That's no longer necessary.
martint
force-pushed
the
time-to-timestamp
branch
from
43bd153 to
1aed95b
Compare
Fixes #5736
Relates to #5738