[SPARK-32424][SQL][3.0] Fix silent data change for timestamp parsing if f overflow happens #29267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yaooqinn
commented
Jul 28, 2020
| @@ -146,7 +146,3 @@ select from_json('{"time":"26/October/2015"}', 'time Timestamp', map('timestampF | |||
| select from_json('{"date":"26/October/2015"}', 'date Date', map('dateFormat', 'dd/MMMMM/yyyy')); | |||
| select from_csv('26/October/2015', 'time Timestamp', map('timestampFormat', 'dd/MMMMM/yyyy')); | |||
| select from_csv('26/October/2015', 'date Date', map('dateFormat', 'dd/MMMMM/yyyy')); | |||
|
|
|||
| select from_unixtime(1, 'yyyyyyyyyyy-MM-dd'); | |||
There was a problem hiding this comment.
I checked these tests which were added to test cases that exceed 10-'y'. It's safe to remove them now because they will fail starting from 7-'y' and we have already covered these in both datetime-parsing.sql and datetime-formatting.sql
|
cc @cloud-fan |
|
Test build #126686 has finished for PR 29267 at commit
|
|
Test build #126690 has finished for PR 29267 at commit
|
|
retest this please |
|
github action passes, thanks, merging to 3.0! |
cloud-fan
pushed a commit
that referenced
this pull request
Jul 28, 2020
…if f overflow happens This PR backports d315ebf to branch-3.0 ### What changes were proposed in this pull request? When using `Seconds.toMicros` API to convert epoch seconds to microseconds, ```scala /** * Equivalent to * {link #convert(long, TimeUnit) MICROSECONDS.convert(duration, this)}. * param duration the duration * return the converted duration, * or {code Long.MIN_VALUE} if conversion would negatively * overflow, or {code Long.MAX_VALUE} if it would positively overflow. */ ``` This PR change it to `Math.multiplyExact(epochSeconds, MICROS_PER_SECOND)` ### Why are the changes needed? fix silent data change between 3.x and 2.x ``` ~/Downloads/spark/spark-3.1.0-SNAPSHOT-bin-20200722 bin/spark-sql -S -e "select to_timestamp('300000', 'y');" +294247-01-10 12:00:54.775807 ``` ``` kentyaohulk ~/Downloads/spark/spark-2.4.5-bin-hadoop2.7 bin/spark-sql -S -e "select to_timestamp('300000', 'y');" 284550-10-19 15:58:1010.448384 ``` ### Does this PR introduce _any_ user-facing change? Yes, we will raise `ArithmeticException` instead of giving the wrong answer if overflow. ### How was this patch tested? add unit test Closes #29267 from yaooqinn/SPARK-32424-30. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
|
Test build #126703 has finished for PR 29267 at commit
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR backports d315ebf to branch-3.0
What changes were proposed in this pull request?
When using
Seconds.toMicrosAPI to convert epoch seconds to microseconds,This PR change it to
Math.multiplyExact(epochSeconds, MICROS_PER_SECOND)Why are the changes needed?
fix silent data change between 3.x and 2.x
Does this PR introduce any user-facing change?
Yes, we will raise
ArithmeticExceptioninstead of giving the wrong answer if overflow.How was this patch tested?
add unit test