Skip to content

[SPARK-29532][SQL] Simplify interval string parsing #26190

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
wants to merge 11 commits into from
Closed

[SPARK-29532][SQL] Simplify interval string parsing #26190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c8d81e3
simplify interval string parsing
cloud-fan Oct 17, 2019
87417d9
address comments
cloud-fan Oct 21, 2019
a5f355f
fix test
cloud-fan Oct 22, 2019
10aa751
re-generate golden file
cloud-fan Oct 22, 2019
5c5e251
address comments
cloud-fan Oct 23, 2019
03e90e4
update benchmark
cloud-fan Oct 23, 2019
836e868
fix
cloud-fan Oct 23, 2019
33ceedc
update
cloud-fan Oct 23, 2019
f759fd5
Regenerated result on EC2 (#14)
dongjoon-hyun Oct 24, 2019
77f69a1
Merge remote-tracking branch 'origin/master' into parser
cloud-fan Oct 24, 2019
48b7ef4
more tests
cloud-fan Oct 24, 2019
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
156 changes: 30 additions & 126 deletions common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,94 +32,11 @@ public final class CalendarInterval implements Serializable {
public static final long MICROS_PER_DAY = MICROS_PER_HOUR * 24;
public static final long MICROS_PER_WEEK = MICROS_PER_DAY * 7;

/**
* A function to generate regex which matches interval string's unit part like "3 years".
*
* First, we can leave out some units in interval string, and we only care about the value of
* unit, so here we use non-capturing group to wrap the actual regex.
* At the beginning of the actual regex, we should match spaces before the unit part.
* Next is the number part, starts with an optional "-" to represent negative value. We use
* capturing group to wrap this part as we need the value later.
* Finally is the unit name, ends with an optional "s".
*/
private static String unitRegex(String unit) {
return "(?:\\s+(-?\\d+)\\s+" + unit + "s?)?";
}

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"),
Pattern.CASE_INSENSITIVE);

private static Pattern yearMonthPattern =
Pattern.compile("^(?:['|\"])?([+|-])?(\\d+)-(\\d+)(?:['|\"])?$");
private static Pattern yearMonthPattern = Pattern.compile(
"^([+|-])?(\\d+)-(\\d+)$");

private static Pattern dayTimePattern = Pattern.compile(
"^(?:['|\"])?([+|-])?((\\d+) )?((\\d+):)?(\\d+):(\\d+)(\\.(\\d+))?(?:['|\"])?$");

private static Pattern quoteTrimPattern = Pattern.compile("^(?:['|\"])?(.*?)(?:['|\"])?$");

private static long toLong(String s) {
if (s == null) {
return 0;
} else {
return Long.parseLong(s);
}
}

/**
* Convert a string to CalendarInterval. Return null if the input string is not a valid interval.
* This method is case-insensitive.
*/
public static CalendarInterval fromString(String s) {
try {
return fromCaseInsensitiveString(s);
} catch (IllegalArgumentException e) {
return null;
}
}

/**
* Convert a string to CalendarInterval. This method can handle
* strings without the `interval` prefix and throws IllegalArgumentException
* when the input string is not a valid interval.
*
* @throws IllegalArgumentException if the string is not a valid internal.
*/
public static CalendarInterval fromCaseInsensitiveString(String s) {
if (s == null) {
throw new IllegalArgumentException("Interval cannot be null");
}
String trimmed = s.trim();
if (trimmed.isEmpty()) {
throw new IllegalArgumentException("Interval cannot be blank");
}
String prefix = "interval";
String intervalStr = trimmed;
// Checks the given interval string does not start with the `interval` prefix
if (!intervalStr.regionMatches(true, 0, prefix, 0, prefix.length())) {
// Prepend `interval` if it does not present because
// the regular expression strictly require it.
intervalStr = prefix + " " + trimmed;
} else if (intervalStr.length() == prefix.length()) {
throw new IllegalArgumentException("Interval string must have time units");
}

Matcher m = p.matcher(intervalStr);
if (!m.matches()) {
throw new IllegalArgumentException("Invalid interval: " + s);
}

long months = toLong(m.group(1)) * 12 + toLong(m.group(2));
long microseconds = toLong(m.group(3)) * MICROS_PER_WEEK;
microseconds += toLong(m.group(4)) * MICROS_PER_DAY;
microseconds += toLong(m.group(5)) * MICROS_PER_HOUR;
microseconds += toLong(m.group(6)) * MICROS_PER_MINUTE;
microseconds += toLong(m.group(7)) * MICROS_PER_SECOND;
microseconds += toLong(m.group(8)) * MICROS_PER_MILLI;
microseconds += toLong(m.group(9));
return new CalendarInterval((int) months, microseconds);
}
"^([+|-])?((\\d+) )?((\\d+):)?(\\d+):(\\d+)(\\.(\\d+))?$");

public static long toLongWithRange(String fieldName,
Copy link
Member

@MaxGekk MaxGekk Oct 23, 2019

Choose a reason for hiding this comment

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

Maybe, throw an exception if s == null instead of to return null?

Copy link
Contributor Author

@cloud-fan cloud-fan Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

maybe it's better, but I'd like to avoid changing unrelated code.

String s, long minValue, long maxValue) throws IllegalArgumentException {
Expand Down Expand Up @@ -242,72 +159,59 @@ public static CalendarInterval fromDayTimeString(String s, String from, String t
return result;
}

public static CalendarInterval fromSingleUnitString(String unit, String s)
public static CalendarInterval fromUnitStrings(String[] units, String[] values)
throws IllegalArgumentException {
assert units.length == values.length;
int months = 0;
long microseconds = 0;

CalendarInterval result = null;
if (s == null) {
throw new IllegalArgumentException(String.format("Interval %s string was null", unit));
}
s = s.trim();
Matcher m = quoteTrimPattern.matcher(s);
if (!m.matches()) {
throw new IllegalArgumentException(
"Interval string does not match day-time format of 'd h:m:s.n': " + s);
} else {
for (int i = 0; i < units.length; i++) {
try {
switch (unit) {
switch (units[i]) {
case "year":
int year = (int) toLongWithRange("year", m.group(1),
Integer.MIN_VALUE / 12, Integer.MAX_VALUE / 12);
result = new CalendarInterval(year * 12, 0L);
months = Math.addExact(months, Math.multiplyExact(Integer.parseInt(values[i]), 12));
break;
case "month":
int month = (int) toLongWithRange("month", m.group(1),
Integer.MIN_VALUE, Integer.MAX_VALUE);
result = new CalendarInterval(month, 0L);
months = Math.addExact(months, Integer.parseInt(values[i]));
break;
case "week":
long week = toLongWithRange("week", m.group(1),
Long.MIN_VALUE / MICROS_PER_WEEK, Long.MAX_VALUE / MICROS_PER_WEEK);
result = new CalendarInterval(0, week * MICROS_PER_WEEK);
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(values[i]), MICROS_PER_WEEK));
break;
case "day":
long day = toLongWithRange("day", m.group(1),
Long.MIN_VALUE / MICROS_PER_DAY, Long.MAX_VALUE / MICROS_PER_DAY);
result = new CalendarInterval(0, day * MICROS_PER_DAY);
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(values[i]), MICROS_PER_DAY));
break;
case "hour":
long hour = toLongWithRange("hour", m.group(1),
Long.MIN_VALUE / MICROS_PER_HOUR, Long.MAX_VALUE / MICROS_PER_HOUR);
result = new CalendarInterval(0, hour * MICROS_PER_HOUR);
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(values[i]), MICROS_PER_HOUR));
break;
case "minute":
long minute = toLongWithRange("minute", m.group(1),
Long.MIN_VALUE / MICROS_PER_MINUTE, Long.MAX_VALUE / MICROS_PER_MINUTE);
result = new CalendarInterval(0, minute * MICROS_PER_MINUTE);
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(values[i]), MICROS_PER_MINUTE));
break;
case "second": {
long micros = parseSecondNano(m.group(1));
result = new CalendarInterval(0, micros);
microseconds = Math.addExact(microseconds, parseSecondNano(values[i]));
break;
}
case "millisecond":
long millisecond = toLongWithRange("millisecond", m.group(1),
Long.MIN_VALUE / MICROS_PER_MILLI, Long.MAX_VALUE / MICROS_PER_MILLI);
result = new CalendarInterval(0, millisecond * MICROS_PER_MILLI);
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(values[i]), MICROS_PER_MILLI));
break;
case "microsecond": {
long micros = Long.parseLong(m.group(1));
result = new CalendarInterval(0, micros);
case "microsecond":
microseconds = Math.addExact(microseconds, Long.parseLong(values[i]));
break;
}
}
} catch (Exception e) {
throw new IllegalArgumentException("Error parsing interval string: " + e.getMessage(), e);
}
}
return result;
return new CalendarInterval(months, microseconds);
}

/**
Expand Down
174 changes: 12 additions & 162 deletions common/unsafe/src/test/java/org/apache/spark/unsafe/types/CalendarIntervalSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@

import org.junit.Test;

import java.util.Arrays;

import static org.junit.Assert.*;
import static org.apache.spark.unsafe.types.CalendarInterval.*;

Expand Down Expand Up @@ -62,72 +60,6 @@ public void toStringTest() {
assertEquals("interval 2 years 10 months 3 weeks 13 hours 123 microseconds", i.toString());
}

@Test
public void fromStringTest() {
testSingleUnit("year", 3, 36, 0);
testSingleUnit("month", 3, 3, 0);
testSingleUnit("week", 3, 0, 3 * MICROS_PER_WEEK);
testSingleUnit("day", 3, 0, 3 * MICROS_PER_DAY);
testSingleUnit("hour", 3, 0, 3 * MICROS_PER_HOUR);
testSingleUnit("minute", 3, 0, 3 * MICROS_PER_MINUTE);
testSingleUnit("second", 3, 0, 3 * MICROS_PER_SECOND);
testSingleUnit("millisecond", 3, 0, 3 * MICROS_PER_MILLI);
testSingleUnit("microsecond", 3, 0, 3);

CalendarInterval result = new CalendarInterval(-5 * 12 + 23, 0);
Arrays.asList(
"interval -5 years 23 month",
" -5 years 23 month",
"interval -5 years 23 month ",
" -5 years 23 month ",
" interval -5 years 23 month ").forEach(input ->
assertEquals(fromString(input), result)
);

// Error cases
Arrays.asList(
"interval 3month 1 hour",
"3month 1 hour",
"interval 3 moth 1 hour",
"3 moth 1 hour",
"interval",
"int",
"",
null).forEach(input -> assertNull(fromString(input)));
}

@Test
public void fromCaseInsensitiveStringTest() {
for (String input : new String[]{"5 MINUTES", "5 minutes", "5 Minutes"}) {
assertEquals(fromCaseInsensitiveString(input), new CalendarInterval(0, 5L * 60 * 1_000_000));
}

for (String input : new String[]{null, "", " "}) {
try {
fromCaseInsensitiveString(input);
fail("Expected to throw an exception for the invalid input");
} catch (IllegalArgumentException e) {
String msg = e.getMessage();
if (input == null) assertTrue(msg.contains("cannot be null"));
else assertTrue(msg.contains("cannot be blank"));
}
}

for (String input : new String[]{"interval", "interval1 day", "foo", "foo 1 day"}) {
try {
fromCaseInsensitiveString(input);
fail("Expected to throw an exception for the invalid input");
} catch (IllegalArgumentException e) {
String msg = e.getMessage();
if (input.trim().equalsIgnoreCase("interval")) {
assertTrue(msg.contains("Interval string must have time units"));
} else {
assertTrue(msg.contains("Invalid interval:"));
}
}
}
}

@Test
public void fromYearMonthStringTest() {
String input;
Expand Down Expand Up @@ -194,107 +126,25 @@ public void fromDayTimeStringTest() {
}
}

@Test
public void fromSingleUnitStringTest() {
String input;
CalendarInterval i;

input = "12";
i = new CalendarInterval(12 * 12, 0L);
assertEquals(fromSingleUnitString("year", input), i);

input = "100";
i = new CalendarInterval(0, 100 * MICROS_PER_DAY);
assertEquals(fromSingleUnitString("day", input), i);

input = "1999.38888";
i = new CalendarInterval(0, 1999 * MICROS_PER_SECOND + 38);
assertEquals(fromSingleUnitString("second", input), i);

try {
input = String.valueOf(Integer.MAX_VALUE);
fromSingleUnitString("year", input);
fail("Expected to throw an exception for the invalid input");
} catch (IllegalArgumentException e) {
assertTrue(e.getMessage().contains("outside range"));
}

try {
input = String.valueOf(Long.MAX_VALUE / MICROS_PER_HOUR + 1);
fromSingleUnitString("hour", input);
fail("Expected to throw an exception for the invalid input");
} catch (IllegalArgumentException e) {
assertTrue(e.getMessage().contains("outside range"));
}
}

@Test
public void addTest() {
String input = "interval 3 month 1 hour";
String input2 = "interval 2 month 100 hour";

CalendarInterval interval = fromString(input);
CalendarInterval interval2 = fromString(input2);

assertEquals(interval.add(interval2), new CalendarInterval(5, 101 * MICROS_PER_HOUR));
CalendarInterval input1 = new CalendarInterval(3, 1 * MICROS_PER_HOUR);
CalendarInterval input2 = new CalendarInterval(2, 100 * MICROS_PER_HOUR);
assertEquals(input1.add(input2), new CalendarInterval(5, 101 * MICROS_PER_HOUR));

input = "interval -10 month -81 hour";
input2 = "interval 75 month 200 hour";

interval = fromString(input);
interval2 = fromString(input2);

assertEquals(interval.add(interval2), new CalendarInterval(65, 119 * MICROS_PER_HOUR));
input1 = new CalendarInterval(-10, -81 * MICROS_PER_HOUR);
input2 = new CalendarInterval(75, 200 * MICROS_PER_HOUR);
assertEquals(input1.add(input2), new CalendarInterval(65, 119 * MICROS_PER_HOUR));
}

@Test
public void subtractTest() {
String input = "interval 3 month 1 hour";
String input2 = "interval 2 month 100 hour";

CalendarInterval interval = fromString(input);
CalendarInterval interval2 = fromString(input2);

assertEquals(interval.subtract(interval2), new CalendarInterval(1, -99 * MICROS_PER_HOUR));

input = "interval -10 month -81 hour";
input2 = "interval 75 month 200 hour";

interval = fromString(input);
interval2 = fromString(input2);

assertEquals(interval.subtract(interval2), new CalendarInterval(-85, -281 * MICROS_PER_HOUR));
}

private static void testSingleUnit(String unit, int number, int months, long microseconds) {
Arrays.asList("interval ", "").forEach(prefix -> {
String input1 = prefix + number + " " + unit;
String input2 = prefix + number + " " + unit + "s";
CalendarInterval result = new CalendarInterval(months, microseconds);
assertEquals(fromString(input1), result);
assertEquals(fromString(input2), result);
});
}

@Test
public void fromStringCaseSensitivityTest() {
testSingleUnit("YEAR", 3, 36, 0);
testSingleUnit("Month", 3, 3, 0);
testSingleUnit("Week", 3, 0, 3 * MICROS_PER_WEEK);
testSingleUnit("DAY", 3, 0, 3 * MICROS_PER_DAY);
testSingleUnit("HouR", 3, 0, 3 * MICROS_PER_HOUR);
testSingleUnit("MiNuTe", 3, 0, 3 * MICROS_PER_MINUTE);
testSingleUnit("Second", 3, 0, 3 * MICROS_PER_SECOND);
testSingleUnit("MilliSecond", 3, 0, 3 * MICROS_PER_MILLI);
testSingleUnit("MicroSecond", 3, 0, 3);

String input;

input = "INTERVAL -5 YEARS 23 MONTHS";
CalendarInterval result = new CalendarInterval(-5 * 12 + 23, 0);
assertEquals(fromString(input), result);
CalendarInterval input1 = new CalendarInterval(3, 1 * MICROS_PER_HOUR);
CalendarInterval input2 = new CalendarInterval(2, 100 * MICROS_PER_HOUR);
assertEquals(input1.subtract(input2), new CalendarInterval(1, -99 * MICROS_PER_HOUR));

assertNull(fromString("INTERVAL"));
assertNull(fromString(" Interval "));
input1 = new CalendarInterval(-10, -81 * MICROS_PER_HOUR);
input2 = new CalendarInterval(75, 200 * MICROS_PER_HOUR);
assertEquals(input1.subtract(input2), new CalendarInterval(-85, -281 * MICROS_PER_HOUR));
}
}
Loading