Skip to content

[Minor][SQL] Allow spaces in the beginning and ending of string for Interval #7390

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.

Sign up for GitHub

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

Closed
viirya wants to merge 3 commits into from
Closed

[Minor][SQL] Allow spaces in the beginning and ending of string for Interval #7390

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
815a9cb
Slightly modify regex to allow spaces in the beginning and ending of …
viirya Jul 14, 2015
57861f7
Fix scala style.
viirya Jul 14, 2015
9eb6831
Use trim instead of modifying regex.
viirya Jul 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions unsafe/src/main/java/org/apache/spark/unsafe/types/Interval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,13 @@ public final class Interval implements Serializable {
* 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*)?";

@srowen srowen Jul 14, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cloud-fan cloud-fan Jul 14, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@srowen srowen Jul 14, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@rxin rxin Jul 14, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@rxin rxin Jul 14, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@srowen srowen Jul 15, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}

private static Pattern p = Pattern.compile("interval" + unitRegex("year") + unitRegex("month") +
unitRegex("week") + unitRegex("day") + unitRegex("hour") + unitRegex("minute") +
unitRegex("second") + unitRegex("millisecond") + unitRegex("microsecond"));
private static Pattern p = Pattern.compile("\\s*interval" + unitRegex("year") +
unitRegex("month") + unitRegex("week") + unitRegex("day") + unitRegex("hour") +
unitRegex("minute") + unitRegex("second") + unitRegex("millisecond") +
unitRegex("microsecond"));

private static long toLong(String s) {
if (s == null) {
Expand Down
6 changes: 6 additions & 0 deletions unsafe/src/test/java/org/apache/spark/unsafe/types/IntervalSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ public void fromStringTest() {
Interval result = new Interval(-5 * 12 + 23, 0);
assertEquals(Interval.fromString(input), result);

input = "interval -5 years 23 month ";
assertEquals(Interval.fromString(input), result);

input = " interval -5 years 23 month ";
assertEquals(Interval.fromString(input), result);

// Error cases
input = "interval 3month 1 hour";
assertEquals(Interval.fromString(input), null);
Expand Down