Skip to content

[SPARK-29486][SQL] CalendarInterval should have 3 fields: months, days and microseconds #26134

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

[SPARK-29486][SQL] CalendarInterval should have 3 fields: months, days and microseconds #26134

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
44d66d6
modify class CalendarInterval and CalendarIntervalSuite
LinhongLiu Oct 11, 2019
4979e1e
modify DateTimeUtils, DateTimeUtilsSuite, TemporalSequenceImpl
LinhongLiu Oct 12, 2019
f09b529
modify CalendarInterval related classes in sql/catalyst
LinhongLiu Oct 12, 2019
0db5b7a
fix test in sql/catalyst
LinhongLiu Oct 12, 2019
ce879c2
fix remaining tests
LinhongLiu Oct 14, 2019
4ee8354
fix failed tests
LinhongLiu Oct 15, 2019
3d954d6
code clean and more bug fix
LinhongLiu Oct 16, 2019
05042e8
address comments and fix tests
LinhongLiu Oct 16, 2019
4c31401
Merge remote-tracking branch 'origin/master' into calendarinterval
LinhongLiu Oct 16, 2019
064be74
fix python
LinhongLiu Oct 18, 2019
211543f
Merge remote-tracking branch 'origin/master' into calendarinterval
LinhongLiu Oct 18, 2019
0778f9a
use 24 hours = 1 day assumption in window, trigger and watermark
LinhongLiu Oct 20, 2019
3a6518a
Merge remote-tracking branch 'origin/master' into calendarinterval
LinhongLiu Oct 20, 2019
4d8383c
streaming changes followup
LinhongLiu Oct 20, 2019
1ac157e
fix conflict
LinhongLiu Oct 20, 2019
aac92bd
Merge remote-tracking branch 'origin/master' into calendarinterval
LinhongLiu Oct 26, 2019
076ce42
fix code sytle and test cases
LinhongLiu Oct 28, 2019
3d62a24
Merge remote-tracking branch 'origin/master' into calendarinterval
LinhongLiu Oct 28, 2019
2edd8a0
change long,long,long to int,int,long when store CanlendarInterval in…
LinhongLiu Oct 28, 2019
4e97bc0
fix comment
LinhongLiu Oct 29, 2019
0e87e2d
Merge remote-tracking branch 'origin/master' into calendarinterval
LinhongLiu Oct 31, 2019
2f90189
address comments
LinhongLiu Nov 1, 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
68 changes: 36 additions & 32 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 @@ -111,14 +111,13 @@ public static CalendarInterval fromCaseInsensitiveString(String 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;
long days = toLong(m.group(3)) * 7 + toLong(m.group(4));
long 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);
return new CalendarInterval((int) months, (int) days, microseconds);
Copy link
Member

@MaxGekk MaxGekk Oct 16, 2019

Choose a reason for hiding this comment

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

I think it is better to catch int overflow by Math.toIntExact instead of silently ignore it.

Copy link
Contributor Author

@LinhongLiu LinhongLiu Oct 16, 2019

Choose a reason for hiding this comment

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

or maybe we can change this to long

Copy link
Member

@MaxGekk MaxGekk Oct 18, 2019

Choose a reason for hiding this comment

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

Valid range of dates/timestamps is [0..10000). So, we should support days up to 10000 * 366 = 3660000. Even int is so much but it is ok.

}

public static long toLongWithRange(String fieldName,
Expand Down Expand Up @@ -154,7 +153,7 @@ public static CalendarInterval fromYearMonthString(String s) throws IllegalArgum
int sign = m.group(1) != null && m.group(1).equals("-") ? -1 : 1;
int years = (int) toLongWithRange("year", m.group(2), 0, Integer.MAX_VALUE);
int months = (int) toLongWithRange("month", m.group(3), 0, 11);
result = new CalendarInterval(sign * (years * 12 + months), 0);
result = new CalendarInterval(sign * (years * 12 + months), 0, 0);
} catch (Exception e) {
throw new IllegalArgumentException(
"Error parsing interval year-month string: " + e.getMessage(), e);
Expand Down Expand Up @@ -195,7 +194,7 @@ public static CalendarInterval fromDayTimeString(String s, String from, String t
} else {
try {
int sign = m.group(1) != null && m.group(1).equals("-") ? -1 : 1;
long days = m.group(2) == null ? 0 : toLongWithRange("day", m.group(3),
int days = m.group(2) == null ? 0 : (int) toLongWithRange("day", m.group(3),
Copy link
Member

@MaxGekk MaxGekk Oct 16, 2019

Choose a reason for hiding this comment

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

Use Math.toIntExact and in other similar places

Copy link
Contributor Author

@LinhongLiu LinhongLiu Oct 20, 2019

Choose a reason for hiding this comment

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

can we use Math.toIntExact for only days field? as you know, there are long -> int in months filed as well, should we cover both of them?

Copy link
Contributor

@cloud-fan cloud-fan Oct 28, 2019

Choose a reason for hiding this comment

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

This is OK here, we call toLongWithRange with Integer.MAX_VALUE so it's safe to cast to int.

0, Integer.MAX_VALUE);
long hours = 0;
long minutes;
Expand Down Expand Up @@ -231,8 +230,8 @@ public static CalendarInterval fromDayTimeString(String s, String from, String t
throw new IllegalArgumentException(
String.format("Cannot support (interval '%s' %s to %s) expression", s, from, to));
}
result = new CalendarInterval(0, sign * (
days * MICROS_PER_DAY + hours * MICROS_PER_HOUR + minutes * MICROS_PER_MINUTE +
result = new CalendarInterval(0, sign * days, sign * (
hours * MICROS_PER_HOUR + minutes * MICROS_PER_MINUTE +
seconds * MICROS_PER_SECOND + nanos / 1000L));
} catch (Exception e) {
throw new IllegalArgumentException(
Expand Down Expand Up @@ -260,46 +259,46 @@ public static CalendarInterval fromSingleUnitString(String unit, String s)
case "year":
int year = (int) toLongWithRange("year", m.group(1),
Integer.MIN_VALUE / 12, Integer.MAX_VALUE / 12);
result = new CalendarInterval(year * 12, 0L);
result = new CalendarInterval(year * 12, 0, 0L);
break;
case "month":
int month = (int) toLongWithRange("month", m.group(1),
Integer.MIN_VALUE, Integer.MAX_VALUE);
result = new CalendarInterval(month, 0L);
result = new CalendarInterval(month, 0, 0L);
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);
int week = (int) toLongWithRange("week", m.group(1),
Integer.MIN_VALUE / 7, Integer.MAX_VALUE / 7);
result = new CalendarInterval(0, week * 7, 0L);
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);
int day = (int) toLongWithRange("day", m.group(1),
Integer.MIN_VALUE, Integer.MAX_VALUE);
result = new CalendarInterval(0, day, 0L);
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);
result = new CalendarInterval(0, 0, hour * 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);
result = new CalendarInterval(0, 0, minute * MICROS_PER_MINUTE);
break;
case "second": {
long micros = parseSecondNano(m.group(1));
result = new CalendarInterval(0, micros);
result = new CalendarInterval(0, 0, micros);
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);
result = new CalendarInterval(0, 0, millisecond * MICROS_PER_MILLI);
break;
case "microsecond": {
long micros = Long.parseLong(m.group(1));
result = new CalendarInterval(0, micros);
result = new CalendarInterval(0, 0, micros);
break;
}
}
Expand Down Expand Up @@ -332,31 +331,35 @@ public static long parseSecondNano(String secondNano) throws IllegalArgumentExce
}

public final int months;
public final int days;
public final long microseconds;

public long milliseconds() {
Copy link
Member

@MaxGekk MaxGekk Oct 16, 2019

Choose a reason for hiding this comment

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

The method will return different values because you exclude days from microseconds

return this.microseconds / MICROS_PER_MILLI;
}

public CalendarInterval(int months, long microseconds) {
public CalendarInterval(int months, int days, long microseconds) {
Copy link
Member

@zsxwing zsxwing Nov 26, 2019

Choose a reason for hiding this comment

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

@LinhongLiu Could you send out a follow up PR to document why we need days and how do we use it in this file? This is definitely worth to document. Otherwise, everyone reading this class may need to git blame and go through the long discussion in this PR.

Copy link
Contributor Author

@LinhongLiu LinhongLiu Nov 27, 2019

Choose a reason for hiding this comment

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

@LinhongLiu Could you send out a follow up PR to document why we need days and how do we use it in this file? This is definitely worth to document. Otherwise, everyone reading this class may need to git blame and go through the long discussion in this PR.

Sure, will do.

this.months = months;
this.days = days;
this.microseconds = microseconds;
}

public CalendarInterval add(CalendarInterval that) {
int months = this.months + that.months;
int days = this.days + that.days;
long microseconds = this.microseconds + that.microseconds;
return new CalendarInterval(months, microseconds);
return new CalendarInterval(months, days, microseconds);
}

public CalendarInterval subtract(CalendarInterval that) {
int months = this.months - that.months;
int days = this.days - that.days;
long microseconds = this.microseconds - that.microseconds;
return new CalendarInterval(months, microseconds);
return new CalendarInterval(months, days, microseconds);
}

public CalendarInterval negate() {
return new CalendarInterval(-this.months, -this.microseconds);
return new CalendarInterval(-this.months, -this.days, -this.microseconds);
}

@Override
Expand All @@ -365,12 +368,12 @@ public boolean equals(Object other) {
if (other == null || !(other instanceof CalendarInterval)) return false;

CalendarInterval o = (CalendarInterval) other;
return this.months == o.months && this.microseconds == o.microseconds;
return this.months == o.months && this.days == o.days && this.microseconds == o.microseconds;
Copy link
Member

@xuanyuanking xuanyuanking Oct 16, 2019

Choose a reason for hiding this comment

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

Is this line changed manually? We'd better use IDE to generate the equals and hashCode.

}

@Override
public int hashCode() {
return 31 * months + (int) microseconds;
return 31 * (31 * months + days) + (int) microseconds;
Copy link
Member

@MaxGekk MaxGekk Oct 16, 2019

Choose a reason for hiding this comment

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

Not related to the PR but I am wondering why the most significant bits of microseconds are completely excluded from the hash.

}

@Override
Expand All @@ -382,12 +385,13 @@ public String toString() {
appendUnit(sb, months % 12, "month");
}

if (days != 0) {
appendUnit(sb, days / 7, "week");
appendUnit(sb, days % 7, "day");
}

if (microseconds != 0) {
long rest = microseconds;
appendUnit(sb, rest / MICROS_PER_WEEK, "week");
rest %= MICROS_PER_WEEK;
appendUnit(sb, rest / MICROS_PER_DAY, "day");
rest %= MICROS_PER_DAY;
appendUnit(sb, rest / MICROS_PER_HOUR, "hour");
rest %= MICROS_PER_HOUR;
appendUnit(sb, rest / MICROS_PER_MINUTE, "minute");
Expand All @@ -397,7 +401,7 @@ public String toString() {
appendUnit(sb, rest / MICROS_PER_MILLI, "millisecond");
rest %= MICROS_PER_MILLI;
appendUnit(sb, rest, "microsecond");
} else if (months == 0) {
} else if (months == 0 && days == 0) {
sb.append(" 0 microseconds");
}

Expand Down
Loading