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

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

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
address comments
  • Loading branch information
@cloud-fan
cloud-fan committed Oct 22, 2019
commit 87417d9e9df227938f386941a17d8a9c8636d2e2
103 changes: 49 additions & 54 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 @@ -159,65 +159,60 @@ public static CalendarInterval fromDayTimeString(String s, String from, String t
return result;
}

public static CalendarInterval fromSingleUnitString(String unit, String s)
public static CalendarInterval fromUnitString(String[] units, String[] values)
Copy link
Member

@maropu maropu Oct 23, 2019

Choose a reason for hiding this comment

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

nit: fromUnitStrings?

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();
try {
switch (unit) {
case "year":
int year = (int) toLongWithRange("year", s,
Integer.MIN_VALUE / 12, Integer.MAX_VALUE / 12);
result = new CalendarInterval(year * 12, 0L);
break;
case "month":
int month = (int) toLongWithRange("month", s,
Integer.MIN_VALUE, Integer.MAX_VALUE);
result = new CalendarInterval(month, 0L);
break;
case "week":
long week = toLongWithRange("week", s,
Long.MIN_VALUE / MICROS_PER_WEEK, Long.MAX_VALUE / MICROS_PER_WEEK);
result = new CalendarInterval(0, week * MICROS_PER_WEEK);
break;
case "day":
long day = toLongWithRange("day", s,
Long.MIN_VALUE / MICROS_PER_DAY, Long.MAX_VALUE / MICROS_PER_DAY);
result = new CalendarInterval(0, day * MICROS_PER_DAY);
break;
case "hour":
long hour = toLongWithRange("hour", s,
Long.MIN_VALUE / MICROS_PER_HOUR, Long.MAX_VALUE / MICROS_PER_HOUR);
result = new CalendarInterval(0, hour * MICROS_PER_HOUR);
break;
case "minute":
long minute = toLongWithRange("minute", s,
Long.MIN_VALUE / MICROS_PER_MINUTE, Long.MAX_VALUE / MICROS_PER_MINUTE);
result = new CalendarInterval(0, minute * MICROS_PER_MINUTE);
break;
case "second": {
long micros = parseSecondNano(s);
result = new CalendarInterval(0, micros);
break;
for (int i = 0; i < units.length; i++) {
try {
String value = values[i].trim();
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.

Is there a case when a value could contain a space?

Copy link
Contributor Author

@cloud-fan cloud-fan Oct 23, 2019

Choose a reason for hiding this comment

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

good question. The trim was here before, but seems the value won't contain space. Let me update it.

switch (units[i]) {
case "year":
months = Math.addExact(months, Math.multiplyExact(Integer.parseInt(value), 12));
break;
case "month":
months = Math.addExact(months, Integer.parseInt(value));
break;
case "week":
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(value), MICROS_PER_WEEK));
break;
case "day":
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(value), MICROS_PER_DAY));
break;
case "hour":
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(value), MICROS_PER_HOUR));
break;
case "minute":
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(value), MICROS_PER_MINUTE));
break;
case "second": {
microseconds = Math.addExact(microseconds, parseSecondNano(value));
break;
}
case "millisecond":
microseconds = Math.addExact(
microseconds,
Math.multiplyExact(Long.parseLong(value), MICROS_PER_MILLI));
break;
case "microsecond":
microseconds = Math.addExact(microseconds, Long.parseLong(value));
break;
}
case "millisecond":
long millisecond = toLongWithRange("millisecond", s,
Long.MIN_VALUE / MICROS_PER_MILLI, Long.MAX_VALUE / MICROS_PER_MILLI);
result = new CalendarInterval(0, millisecond * MICROS_PER_MILLI);
break;
case "microsecond":
long micros = Long.parseLong(s);
result = new CalendarInterval(0, micros);
break;
} catch (Exception e) {
throw new IllegalArgumentException("Error parsing interval string: " + e.getMessage(), e);
}
} catch (Exception e) {
throw new IllegalArgumentException("Error parsing interval string: " + e.getMessage(), e);
}
return result;
return new CalendarInterval(months, microseconds);
}

/**
Expand Down
2 changes: 0 additions & 2 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
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ singleTableSchema
;

singleInterval
: INTERVAL? intervalField+ EOF
: INTERVAL? (intervalValue intervalUnit)+ EOF
;

statement
Expand Down
32 changes: 20 additions & 12 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,11 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging

override def visitSingleInterval(ctx: SingleIntervalContext): CalendarInterval = {
withOrigin(ctx) {
val intervals = ctx.intervalField.asScala.map(visitIntervalField)
validate(intervals.nonEmpty,
"at least one time unit should be given for interval literal", ctx)
intervals.reduce(_.add(_))
val units = ctx.intervalUnit().asScala.map {
u => normalizeInternalUnit(u.getText.toLowerCase(Locale.ROOT))
}.toArray
val values = ctx.intervalValue().asScala.map(getIntervalValue).toArray
CalendarInterval.fromUnitString(units, values)
}
}

Expand Down Expand Up @@ -1940,18 +1941,12 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
*/
override def visitIntervalField(ctx: IntervalFieldContext): CalendarInterval = withOrigin(ctx) {
import ctx._
val s = if (value.STRING() != null) {
string(value.STRING())
} else {
value.getText
}
val s = getIntervalValue(value)
try {
val unitText = unit.getText.toLowerCase(Locale.ROOT)
val interval = (unitText, Option(to).map(_.getText.toLowerCase(Locale.ROOT))) match {
case (u, None) =>
// Handle plural forms, e.g: yearS/monthS/weekS/dayS/hourS/minuteS/hourS/...
val unit = if (u.endsWith("s")) u.substring(0, u.length - 1) else u
CalendarInterval.fromSingleUnitString(unit, s)
CalendarInterval.fromUnitString(Array(normalizeInternalUnit(u)), Array(s))
Copy link
Contributor Author

@cloud-fan cloud-fan Oct 21, 2019

Choose a reason for hiding this comment

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

We can improve this later, by separating the parser rules for 1 year 10 months and '1-10' year to month.

case ("year", Some("month")) =>
CalendarInterval.fromYearMonthString(s)
case ("day", Some("hour")) =>
Expand Down Expand Up @@ -1980,6 +1975,19 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
}
}

private def getIntervalValue(value: IntervalValueContext): String = {
if (value.STRING() != null) {
string(value.STRING())
} else {
value.getText
}
}

// Handle plural forms, e.g: yearS/monthS/weekS/dayS/hourS/minuteS/hourS/...
private def normalizeInternalUnit(s: String): String = {
if (s.endsWith("s")) s.substring(0, s.length - 1) else s
}

/* ********************************************************************************************
* DataType parsing
* ******************************************************************************************** */
Expand Down
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParserInterface.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,6 @@ trait ParserInterface {
/**
* Parse a string to a [[CalendarInterval]].
*/
@throws[ParseException]("Text cannot be parsed to a DataType")
@throws[ParseException]("Text cannot be parsed to an interval")
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.

ParseDriver.parseInterval -> astBuilder.visitSingleInterval -> CalendarInterval.fromUnitString -> throws IllegalArgumentException . I haven't found when it can be converted to ParseException.

Copy link
Contributor Author

@cloud-fan cloud-fan Oct 23, 2019

Choose a reason for hiding this comment

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

thanks for catching! Fixed.

def parseInterval(sqlText: String): CalendarInterval
Copy link
Member

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

@cloud-fan . This looks like a breaking change to the existing Parser Spark Extension. If then, this need a new JIRA. Can we have this change in a separate JIRA and PR because this is an open developer API?

Copy link
Contributor Author

@cloud-fan cloud-fan Oct 23, 2019

Choose a reason for hiding this comment

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

I didn't realize that it's a developer API. I think it's fine to update the developer APIs, and we already did it when adding parseMultipartIdentifier.

Since this PR doesn't need to access ParserInterface directly, let me avoid changing ParserInterface

Copy link
Member

@dongjoon-hyun dongjoon-hyun Oct 23, 2019

Choose a reason for hiding this comment

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

Thanks! Of course, we can change it~ What we need is just having two separate PRs to distinguish them clearly.

}
2 changes: 1 addition & 1 deletion sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ class ExpressionParserSuite extends AnalysisTest {
"microsecond")

def intervalLiteral(u: String, s: String): Literal = {
Literal(CalendarInterval.fromSingleUnitString(u, s))
Literal(CalendarInterval.fromUnitString(Array(u), Array(s)))
}

test("intervals") {
Expand Down