-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-8944][SQL] Support casting between IntervalType and StringType #7355
Conversation
cc @rxin |
String s; | ||
Interval i; | ||
|
||
s = "interval 2 weeks -6 minute"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a unit test where there is only # month specified, but nothing else.
and another one with only # year.
and then one more for year + month.
and one for only day, week, ... all the units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also add a unit test where the string is just "int", and one for empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to test all the combinations of units?
Looks good other than documentation / test cases. I'm assuming code gen is left for the next task for cast? |
Ah sorry forgot the code gen part. Will add it and the tests, documentation improvement. |
|
||
s = "interval -5 years 23 month"; | ||
i = new Interval(-5 * 12 + 23, 0); | ||
assertEquals(Interval.fromString(s), i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to test all the combinations of units?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not - but we should definitely test each unit at least once.
Test build #37085 has finished for PR 7355 at commit
|
Test build #37087 has finished for PR 7355 at commit
|
assertEquals(Interval.fromString(s), i); | ||
|
||
// Error cases | ||
i = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove i and use null directly.
"i" is a bad name since it is usually used for iterating over an array.
lgtm |
Test build #37114 has finished for PR 7355 at commit
|
Test build #37123 has finished for PR 7355 at commit
|
Thanks - I've merged this. |
Author: Wenchen Fan <cloud0fan@outlook.com> Closes apache#7355 from cloud-fan/fromString and squashes the following commits: 3bbb9d6 [Wenchen Fan] fix code gen 7dab957 [Wenchen Fan] naming fix 0fbbe19 [Wenchen Fan] address comments ac1f3d1 [Wenchen Fan] Support casting between IntervalType and StringType
…nterval This is a minor fixing for #7355 to allow spaces in the beginning and ending of string parsed to `Interval`. Author: Liang-Chi Hsieh <viirya@appier.com> Closes #7390 from viirya/fix_interval_string and squashes the following commits: 9eb6831 [Liang-Chi Hsieh] Use trim instead of modifying regex. 57861f7 [Liang-Chi Hsieh] Fix scala style. 815a9cb [Liang-Chi Hsieh] Slightly modify regex to allow spaces in the beginning and ending of string.
No description provided.