[Minor][SQL] Allow spaces in the beginning and ending of string for Interval #7390
Conversation
| @@ -43,12 +43,13 @@ | |||
| * Finally is the unit name, ends with an optional "s". | |||
| */ | |||
| private static String unitRegex(String unit) { | |||
| return "(?:\\s+(-?\\d+)\\s+" + unit + "s?)?"; | |||
| return "(?:\\s+(-?\\d+)\\s+" + unit + "s?\\s*)?"; | |||
There was a problem hiding this comment.
Sort-of related question -- why is this class in 'unsafe', and why not use Joda time? the JDK already has a rich set of abstractions for time. It seems like it's only used in catalyst.
There was a problem hiding this comment.
This class is the internal representation of interval type, written in Java for better performance, just like what we did for UTF8String, that's why it's in 'unsafe'.
Currently our date and timestamp internal representation is int and long, which is more efficient than Joda time I think.
There was a problem hiding this comment.
@cloud-fan maybe I'm blind but how does this use Unsafe? I don't see how it benefits.
java.time.Duration is an int + long too. Is this really needed? at least, why in unsafe?
There was a problem hiding this comment.
@srowen don't hang up on the package name for now. It's just a place to put all the data types implemented in Java. We will most likely rename the package.
There was a problem hiding this comment.
One possible thing that could happen with unsafe for interval is to support reading directly from a memory address rather than materializing it as a long + an int.
There was a problem hiding this comment.
@rxin I'm not worried about the package so much as the module; this doesn't have to do with bit-twiddling via Unsafe. Is there really a win for managing 96 bits via Unsafe?
It doesn't seem like the right place for this type; it's only used by catalyst (now) I think, so could live there or at least core. But more than that, the JDK itself has a standard class for this -- around which you could build utility functions. I understand this is in flux but I suppose I'm not seeing why it landed here and wondering if it's better to move it early.
|
Test build #37207 has finished for PR 7390 at commit
|
|
instead of changing the regex, can we just call |
|
Ok. Update later. |
|
Test build #37228 has finished for PR 7390 at commit
|
|
Jenkins, retest this please. |
|
Test build #37258 has finished for PR 7390 at commit
|
|
Thanks - I've merged this. |
This is a minor fixing for #7355 to allow spaces in the beginning and ending of string parsed to
Interval.