Skip to content
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

SQL: Fix milliseconds handling in intervals #51675

Merged
merged 1 commit into from
Feb 3, 2020
Merged

SQL: Fix milliseconds handling in intervals #51675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/util/DateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;


//FIXME: Taken from sql-proto.
//FIXME: Taken from sql-proto (StringUtils)
//Ideally it should be shared but the dependencies across projects and and SQL-client make it tricky.
// Maybe a gradle task would fix that...
public class DateUtils {
Expand Down Expand Up @@ -136,8 +136,14 @@ public static String toString(Object value) {
sb.append(":");
durationInSec = durationInSec % SECONDS_PER_MINUTE;
sb.append(indent(durationInSec));
sb.append(".");
sb.append(TimeUnit.NANOSECONDS.toMillis(d.getNano()));
long millis = TimeUnit.NANOSECONDS.toMillis(d.getNano());
if (millis > 0) {
sb.append(".");
while (millis % 10 == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively you could just iterate on the string millis from end->start and remove all zeroes.
Don't know if performance wise makes sense...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also fluttered a bit between the two approaches. I went with this approach since generally arithmetic operations should be faster. But anyways, micro-optimisations.

millis /= 10;
}
sb.append(millis);
}
return sb.toString();
}

Expand Down
16 changes: 8 additions & 8 deletions x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/SqlProtocolTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,22 +95,22 @@ public void testIPs() throws IOException {
public void testDateTimeIntervals() throws IOException {
assertQuery("SELECT INTERVAL '326' YEAR", "INTERVAL '326' YEAR", "interval_year", "P326Y", "+326-0", 7);
assertQuery("SELECT INTERVAL '50' MONTH", "INTERVAL '50' MONTH", "interval_month", "P50M", "+0-50", 7);
assertQuery("SELECT INTERVAL '520' DAY", "INTERVAL '520' DAY", "interval_day", "PT12480H", "+520 00:00:00.0", 23);
assertQuery("SELECT INTERVAL '163' HOUR", "INTERVAL '163' HOUR", "interval_hour", "PT163H", "+6 19:00:00.0", 23);
assertQuery("SELECT INTERVAL '163' MINUTE", "INTERVAL '163' MINUTE", "interval_minute", "PT2H43M", "+0 02:43:00.0", 23);
assertQuery("SELECT INTERVAL '223.16' SECOND", "INTERVAL '223.16' SECOND", "interval_second", "PT3M43.016S", "+0 00:03:43.16", 23);
assertQuery("SELECT INTERVAL '520' DAY", "INTERVAL '520' DAY", "interval_day", "PT12480H", "+520 00:00:00", 23);
assertQuery("SELECT INTERVAL '163' HOUR", "INTERVAL '163' HOUR", "interval_hour", "PT163H", "+6 19:00:00", 23);
assertQuery("SELECT INTERVAL '163' MINUTE", "INTERVAL '163' MINUTE", "interval_minute", "PT2H43M", "+0 02:43:00", 23);
assertQuery("SELECT INTERVAL '223.16' SECOND", "INTERVAL '223.16' SECOND", "interval_second", "PT3M43.16S", "+0 00:03:43.16", 23);
assertQuery("SELECT INTERVAL '163-11' YEAR TO MONTH", "INTERVAL '163-11' YEAR TO MONTH", "interval_year_to_month", "P163Y11M",
"+163-11", 7);
assertQuery("SELECT INTERVAL '163 12' DAY TO HOUR", "INTERVAL '163 12' DAY TO HOUR", "interval_day_to_hour", "PT3924H",
"+163 12:00:00.0", 23);
"+163 12:00:00", 23);
assertQuery("SELECT INTERVAL '163 12:39' DAY TO MINUTE", "INTERVAL '163 12:39' DAY TO MINUTE", "interval_day_to_minute",
"PT3924H39M", "+163 12:39:00.0", 23);
"PT3924H39M", "+163 12:39:00", 23);
assertQuery("SELECT INTERVAL '163 12:39:59.163' DAY TO SECOND", "INTERVAL '163 12:39:59.163' DAY TO SECOND",
"interval_day_to_second", "PT3924H39M59.163S", "+163 12:39:59.163", 23);
assertQuery("SELECT INTERVAL -'163 23:39:56.23' DAY TO SECOND", "INTERVAL -'163 23:39:56.23' DAY TO SECOND",
"interval_day_to_second", "PT-3935H-39M-56.023S", "-163 23:39:56.23", 23);
"interval_day_to_second", "PT-3935H-39M-56.23S", "-163 23:39:56.23", 23);
assertQuery("SELECT INTERVAL '163:39' HOUR TO MINUTE", "INTERVAL '163:39' HOUR TO MINUTE", "interval_hour_to_minute",
"PT163H39M", "+6 19:39:00.0", 23);
"PT163H39M", "+6 19:39:00", 23);
assertQuery("SELECT INTERVAL '163:39:59.163' HOUR TO SECOND", "INTERVAL '163:39:59.163' HOUR TO SECOND", "interval_hour_to_second",
"PT163H39M59.163S", "+6 19:39:59.163", 23);
assertQuery("SELECT INTERVAL '163:59.163' MINUTE TO SECOND", "INTERVAL '163:59.163' MINUTE TO SECOND", "interval_minute_to_second",
Expand Down
54 changes: 27 additions & 27 deletions x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@
exactIntervals
SELECT INTERVAL 1 YEAR AS y, INTERVAL 2 MONTH AS m, INTERVAL 3 DAY AS d, INTERVAL 4 HOUR AS h, INTERVAL 5 MINUTE AS mm, INTERVAL 6 SECOND AS s;

y | m | d | h | mm | s
---------------+---------------+---------------+---------------+---------------+---------------
+1-0 |+0-2 |+3 00:00:00.0 |+0 04:00:00.0 |+0 00:05:00.0 |+0 00:00:06.0
y | m | d | h | mm | s
---------------+---------------+-------------+-------------+-------------+-------------
+1-0 |+0-2 |+3 00:00:00 |+0 04:00:00 |+0 00:05:00 |+0 00:00:06
;

testExactIntervalPlural
SELECT INTERVAL 1 YEARS AS y, INTERVAL 2 MONTHS AS m, INTERVAL 3 DAYS AS d, INTERVAL 4 HOURS AS h, INTERVAL 5 MINUTES AS mm, INTERVAL 6 SECONDS AS s;

y | m | d | h | mm | s
---------------+---------------+---------------+---------------+---------------+---------------
+1-0 |+0-2 |+3 00:00:00.0 |+0 04:00:00.0 |+0 00:05:00.0 |+0 00:00:06.0
y | m | d | h | mm | s
---------------+---------------+-------------+-------------+-------------+-------------
+1-0 |+0-2 |+3 00:00:00 |+0 04:00:00 |+0 00:05:00 |+0 00:00:06
;

// take the examples from https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/interval-literals?view=sql-server-2017
Expand All @@ -43,23 +43,23 @@ SELECT INTERVAL '3261' DAY;

INTERVAL '3261' DAY
-------------------
+3261 00:00:00.0
+3261 00:00:00
;

hour
SELECT INTERVAL '163' HOUR;

INTERVAL '163' HOUR
-------------------
+6 19:00:00.0
+6 19:00:00
;

minute
SELECT INTERVAL '163' MINUTE;

INTERVAL '163' MINUTE
---------------------
+0 02:43:00.0
+0 02:43:00
;

second
Expand All @@ -83,15 +83,15 @@ SELECT INTERVAL '163 12' DAY TO HOUR;

INTERVAL '163 12' DAY TO HOUR
-----------------------------
+163 12:00:00.0
+163 12:00:00
;

dayMinute
SELECT INTERVAL '163 12:39' DAY TO MINUTE AS interval;

interval
---------------
+163 12:39:00.0
+163 12:39:00
;

daySecond
Expand All @@ -115,7 +115,7 @@ SELECT INTERVAL '163:39' HOUR TO MINUTE AS interval;

interval
---------------
+6 19:39:00.0
+6 19:39:00
;

hourSecond
Expand All @@ -139,7 +139,7 @@ SELECT INTERVAL 1 DAY + INTERVAL 53 MINUTES;

INTERVAL 1 DAY + INTERVAL 53 MINUTES
------------------------------------
+1 00:53:00.0
+1 00:53:00
;

datePlusIntervalInline
Expand All @@ -162,9 +162,9 @@ SELECT - INTERVAL '49-1' YEAR TO MONTH result;
intervalMinusInterval
SELECT INTERVAL '1' DAY - INTERVAL '2' HOURS AS result;

result
---------------
+0 22:00:00.0
result
-------------
+0 22:00:00
;


Expand All @@ -179,16 +179,16 @@ SELECT -2 * INTERVAL '3' YEARS AS result;
intervalDayMultiply
SELECT -2 * INTERVAL '1 23:45' DAY TO MINUTES AS result;

result
---------------
-3 23:30:00.0
result
-------------
-3 23:30:00
;

intervalHoursMultiply
SELECT 4 * -INTERVAL '2' HOURS AS result1, -5 * -INTERVAL '3' HOURS AS result2;
result1 | result2
---------------+--------------
-0 08:00:00.0 | +0 15:00:00.0
result1 | result2
-------------+------------
-0 08:00:00 | +0 15:00:00
;

intervalNullMath
Expand All @@ -206,11 +206,11 @@ SELECT languages, CAST (languages * INTERVAL '1 10:30' DAY TO MINUTES AS string)

languages | result
---------------+---------------------------------------------
2 | +2 21:00:00.0
5 | +7 04:30:00.0
4 | +5 18:00:00.0
5 | +7 04:30:00.0
1 | +1 10:30:00.0
2 | +2 21:00:00
5 | +7 04:30:00
4 | +5 18:00:00
5 | +7 04:30:00
1 | +1 10:30:00
;

dateMinusInterval
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugin/sql/qa/src/main/resources/docs/docs.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,9 +876,9 @@ dtIntervalPlusInterval
// tag::dtIntervalPlusInterval
SELECT INTERVAL 1 DAY + INTERVAL 53 MINUTES AS result;

result
result
---------------
+1 00:53:00.0
+1 00:53:00

// end::dtIntervalPlusInterval
;
Expand Down Expand Up @@ -909,9 +909,9 @@ dtIntervalMinusInterval
// tag::dtIntervalMinusInterval
SELECT INTERVAL '1' DAY - INTERVAL '2' HOURS AS result;

result
result
---------------
+0 22:00:00.0
+0 22:00:00
// end::dtIntervalMinusInterval
;

Expand Down
10 changes: 8 additions & 2 deletions x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,14 @@ public static String toString(Object value) {
sb.append(":");
durationInSec = durationInSec % SECONDS_PER_MINUTE;
sb.append(indent(durationInSec));
sb.append(".");
sb.append(TimeUnit.NANOSECONDS.toMillis(d.getNano()));
long millis = TimeUnit.NANOSECONDS.toMillis(d.getNano());
if (millis > 0) {
sb.append(".");
while (millis % 10 == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

millis /= 10;
}
sb.append(millis);
}
return sb.toString();
}

Expand Down
4 changes: 4 additions & 0 deletions .../sql/src/main/java/org/elasticsearch/xpack/sql/expression/literal/interval/Intervals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,10 @@ TemporalAmount parse(Source source, String string) {
+ ": negative value [{}] not allowed (negate the entire interval instead)",
v);
}
if (units.get(unitIndex) == TimeUnit.MILLISECOND && number.length() < 3) {
// normalize the number past DOT to millis
v *= number.length() < 2 ? 100 : 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concept here, you could avoid the multiplication by creating a string with right zero padding and then read is as number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be changed later if we ever bring the nanos to intervals and a multiplication might be more succinct. But yes, also here debatable.

}
values[unitIndex++] = v;
} catch (QlIllegalArgumentException siae) {
throw new ParsingException(source, invalidIntervalMessage(string), siae.getMessage());
Expand Down
9 changes: 5 additions & 4 deletions ...src/test/java/org/elasticsearch/xpack/sql/expression/literal/interval/IntervalsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ public void testMinuteInterval() throws Exception {
public void testSecondInterval() throws Exception {
int randomSeconds = randomNonNegativeInt();
int randomMillis = randomBoolean() ? (randomBoolean() ? 0 : 999) : randomInt(999);
String value = format(Locale.ROOT, "%s%d.%d", sign, randomSeconds, randomMillis);
String value = format(Locale.ROOT, "%s%d", sign, randomSeconds);
value += randomMillis > 0 ? format(Locale.ROOT, ".%03d", randomMillis) : "";
TemporalAmount amount = parseInterval(EMPTY, value, INTERVAL_SECOND);
assertEquals(maybeNegate(sign, Duration.ofSeconds(randomSeconds).plusMillis(randomMillis)), amount);
}
Expand Down Expand Up @@ -129,7 +130,7 @@ public void testDayToSecond() throws Exception {

boolean withMillis = randomBoolean();
int randomMilli = withMillis ? randomInt(999) : 0;
String millisString = withMillis ? "." + randomMilli : "";
String millisString = withMillis && randomMilli > 0 ? format(Locale.ROOT, ".%03d", randomMilli) : "";

String value = format(Locale.ROOT, "%s%d %d:%d:%d%s", sign, randomDay, randomHour, randomMinute, randomSecond, millisString);
TemporalAmount amount = parseInterval(EMPTY, value, INTERVAL_DAY_TO_SECOND);
Expand All @@ -152,7 +153,7 @@ public void testHourToSecond() throws Exception {

boolean withMillis = randomBoolean();
int randomMilli = withMillis ? randomInt(999) : 0;
String millisString = withMillis ? "." + randomMilli : "";
String millisString = withMillis && randomMilli > 0 ? format(Locale.ROOT, ".%03d", randomMilli) : "";

String value = format(Locale.ROOT, "%s%d:%d:%d%s", sign, randomHour, randomMinute, randomSecond, millisString);
TemporalAmount amount = parseInterval(EMPTY, value, INTERVAL_HOUR_TO_SECOND);
Expand All @@ -166,7 +167,7 @@ public void testMinuteToSecond() throws Exception {

boolean withMillis = randomBoolean();
int randomMilli = withMillis ? randomInt(999) : 0;
String millisString = withMillis ? "." + randomMilli : "";
String millisString = withMillis && randomMilli > 0 ? format(Locale.ROOT, ".%03d", randomMilli) : "";

String value = format(Locale.ROOT, "%s%d:%d%s", sign, randomMinute, randomSecond, millisString);
TemporalAmount amount = parseInterval(EMPTY, value, INTERVAL_MINUTE_TO_SECOND);
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public void testStringInterval() {
int randomSecond = randomInt(59);
int randomMilli = randomInt(999);

String value = format(Locale.ROOT, "INTERVAL '%d %d:%d:%d.%d' DAY TO SECOND", randomDay, randomHour, randomMinute, randomSecond,
String value = format(Locale.ROOT, "INTERVAL '%d %d:%d:%d.%03d' DAY TO SECOND", randomDay, randomHour, randomMinute, randomSecond,
randomMilli);
assertEquals(Duration.ofDays(randomDay).plusHours(randomHour).plusMinutes(randomMinute).plusSeconds(randomSecond)
.plusMillis(randomMilli), intervalOf(value));
Expand All @@ -172,7 +172,7 @@ public void testNegativeStringInterval() {
int randomSecond = randomInt(59);
int randomMilli = randomInt(999);

String value = format(Locale.ROOT, "INTERVAL -'%d %d:%d:%d.%d' DAY TO SECOND", randomDay, randomHour, randomMinute, randomSecond,
String value = format(Locale.ROOT, "INTERVAL -'%d %d:%d:%d.%03d' DAY TO SECOND", randomDay, randomHour, randomMinute, randomSecond,
randomMilli);
assertEquals(Duration.ofDays(randomDay).plusHours(randomHour).plusMinutes(randomMinute).plusSeconds(randomSecond)
.plusMillis(randomMilli).negated(), intervalOf(value));
Expand Down