[SPARK-29651][SQL][2.4] Fix parsing of interval seconds fraction #26355
Conversation
This file contains hidden or 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
|
Test build #113081 has finished for PR 26355 at commit
|
|
Test build #113083 has finished for PR 26355 at commit
|
Member
Author
|
ping @cloud-fan |
srowen
approved these changes
Nov 1, 2019
Contributor
|
thanks, merging to 2.4! |
cloud-fan
pushed a commit
that referenced
this pull request
Nov 1, 2019
### What changes were proposed in this pull request? In the PR, I propose to extract parsing of the seconds interval units to the private method `parseNanos` in `CalendarInterval` and modify the code to correctly parse the fractional part of the seconds unit of intervals in the cases: - When the fractional part has less than 9 digits - The seconds unit is negative This is a back port of the commit 3206a99. ### Why are the changes needed? The changes are needed to fix the issues: ```sql spark-sql> select interval 10.123456 seconds; interval 10 seconds 123 microseconds ``` The correct result must be `interval 10 seconds 123 milliseconds 456 microseconds` ```sql spark-sql> select interval -10.123456789 seconds; interval -9 seconds -876 milliseconds -544 microseconds ``` but the whole interval should be negated, and the result must be `interval -10 seconds -123 milliseconds -456 microseconds`, taking into account the truncation to microseconds. ### Does this PR introduce any user-facing change? Yes. After changes: ```sql spark-sql> select interval 10.123456 seconds; interval 10 seconds 123 milliseconds 456 microseconds spark-sql> select interval -10.123456789 seconds; interval -10 seconds -123 milliseconds -456 microseconds ``` ### How was this patch tested? By existing test suite, `literals.sql` and new tests in `ExpressionParserSuite`. Closes #26355 from MaxGekk/fix-interval-nanos-parsing-2.4. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
Member
|
Thank you all! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
What changes were proposed in this pull request?
In the PR, I propose to extract parsing of the seconds interval units to the private method
parseNanosinCalendarIntervaland modify the code to correctly parse the fractional part of the seconds unit of intervals in the cases:This is a back port of the commit 3206a99.
Why are the changes needed?
The changes are needed to fix the issues:
The correct result must be
interval 10 seconds 123 milliseconds 456 microsecondsbut the whole interval should be negated, and the result must be
interval -10 seconds -123 milliseconds -456 microseconds, taking into account the truncation to microseconds.Does this PR introduce any user-facing change?
Yes. After changes:
How was this patch tested?
By existing test suite,
literals.sqland new tests inExpressionParserSuite.