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

Jump to bottom

SQL: Replace joda with java time #38437

Merged
merged 9 commits into from
Feb 8, 2019
Merged

SQL: Replace joda with java time #38437

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
af51ec6
SQL: Replace joda with java time
matriv Feb 5, 2019
bc77167
Address comments
matriv Feb 6, 2019
88ef084
Use the date_optional_time formatter instead of a custom one
matriv Feb 6, 2019
459c8db
fix checkstyle
matriv Feb 6, 2019
904c0c8
Merge remote-tracking branch 'upstream/master' into mt/fix-37703
matriv Feb 6, 2019
8e06556
Merge remote-tracking branch 'upstream/master' into mt/fix-37703
matriv Feb 6, 2019
cf9dc6b
Merge remote-tracking branch 'upstream/master' into mt/fix-37703
matriv Feb 6, 2019
e37ddc5
Merge remote-tracking branch 'upstream/master' into mt/fix-37703
matriv Feb 6, 2019
153fa09
Merge remote-tracking branch 'upstream/master' into mt/fix-37703
matriv Feb 8, 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
17 changes: 7 additions & 10 deletions ...ck/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/TypeConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,18 @@
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.test.ESTestCase;
import org.joda.time.DateTime;
import org.joda.time.ReadableDateTime;

import java.sql.Timestamp;
import java.time.Clock;
import java.time.Duration;
import java.time.ZoneId;
import java.time.ZonedDateTime;

import static org.hamcrest.Matchers.instanceOf;


public class TypeConverterTests extends ESTestCase {


public void testFloatAsNative() throws Exception {
assertThat(convertAsNative(42.0f, EsType.FLOAT), instanceOf(Float.class));
assertThat(convertAsNative(42.0, EsType.FLOAT), instanceOf(Float.class));
Expand All @@ -40,21 +41,17 @@ public void testDoubleAsNative() throws Exception {
}

public void testTimestampAsNative() throws Exception {
DateTime now = DateTime.now();
ZonedDateTime now = ZonedDateTime.now(Clock.tick(Clock.system(ZoneId.of("Z")), Duration.ofMillis(1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

why using Clock here?
maybe we could just use
ZonedDateTime.now(ZoneOffset.UTC)

Copy link
Member

Choose a reason for hiding this comment

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

If I recall correctly, it's because the clock approach had unpredictable behavior between jdk 8 and 11.
@astefan do you remember?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right - you guys made an excellent fix here #38437 (comment)
I had similar problems this morning with watcher.

Copy link
Contributor

Choose a reason for hiding this comment

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

@costin yes. We had this issue in the past for the NOW() function - #36877. @matriv's approach is spot on.

Copy link
Contributor

Choose a reason for hiding this comment

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

The same approach is used here maybe you can re-use that method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, ZoneId.of("Z") seems to have a constant defined in DateUtils. Can you use that one or is not visible in this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jdbc module doesn't have access to DateUtils or TestUtils so both are not possible.

assertThat(convertAsNative(now, EsType.DATETIME), instanceOf(Timestamp.class));
assertEquals(now.getMillis(), ((Timestamp) convertAsNative(now, EsType.DATETIME)).getTime());
assertEquals(now.toInstant().toEpochMilli(), ((Timestamp) convertAsNative(now, EsType.DATETIME)).getTime());
}

private Object convertAsNative(Object value, EsType type) throws Exception {
// Simulate sending over XContent
XContentBuilder builder = JsonXContent.contentBuilder();
builder.startObject();
builder.field("value");
if (value instanceof ReadableDateTime) {
builder.value(((ReadableDateTime) value).getMillis());
} else {
builder.value(value);
}
builder.value(value);
builder.endObject();
builder.close();
Object copy = XContentHelper.convertToMap(BytesReference.bytes(builder), false, builder.contentType()).v2().get("value");
Expand Down
5 changes: 0 additions & 5 deletions ...c/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.util.DateUtils;
import org.joda.time.DateTime;

import java.io.IOException;
import java.util.ArrayDeque;
Expand Down Expand Up @@ -132,10 +131,6 @@ private Object unwrapMultiValue(Object values) {
if (values instanceof String) {
return DateUtils.asDateTime(Long.parseLong(values.toString()));
}
// returned by nested types...
Copy link
Contributor Author

@matriv matriv Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@costin I'm not sure about this, but doesn't seem to break something. Should we check more in depth and add tests?

Copy link
Member

Choose a reason for hiding this comment

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

It can be removed now with Joda out. Likely this will eliminate asDateTime as well.

if (values instanceof DateTime) {
return DateUtils.asDateTime((DateTime) values);
}
}
if (values instanceof Long || values instanceof Double || values instanceof String || values instanceof Boolean) {
return values;
Expand Down
33 changes: 12 additions & 21 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,25 +111,25 @@
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.DataTypeConversion;
import org.elasticsearch.xpack.sql.type.DataTypes;
import org.elasticsearch.xpack.sql.util.DateUtils;
import org.elasticsearch.xpack.sql.util.StringUtils;
import org.joda.time.DateTime;
import org.joda.time.format.DateTimeFormatter;
import org.joda.time.format.DateTimeFormatterBuilder;
import org.joda.time.format.ISODateTimeFormat;

import java.time.Duration;
import java.time.LocalTime;
import java.time.Period;
import java.time.format.DateTimeParseException;
import java.time.temporal.TemporalAmount;
import java.util.EnumSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.StringJoiner;

import static java.time.format.DateTimeFormatter.ISO_LOCAL_TIME;
import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static org.elasticsearch.xpack.sql.type.DataTypeConversion.conversionFor;
import static org.elasticsearch.xpack.sql.util.DateUtils.asDateOnly;
import static org.elasticsearch.xpack.sql.util.DateUtils.ofEscapedLiteral;

abstract class ExpressionBuilder extends IdentifierBuilder {

Expand Down Expand Up @@ -791,13 +791,11 @@ public Literal visitDateEscapedLiteral(DateEscapedLiteralContext ctx) {
String string = string(ctx.string());
Source source = source(ctx);
// parse yyyy-MM-dd
DateTime dt = null;
try {
dt = ISODateTimeFormat.date().parseDateTime(string);
} catch(IllegalArgumentException ex) {
return new Literal(source, asDateOnly(string), DataType.DATE);
} catch(DateTimeParseException ex) {
throw new ParsingException(source, "Invalid date received; {}", ex.getMessage());
}
return new Literal(source, DateUtils.asDateOnly(dt), DataType.DATE);
}

@Override
Expand All @@ -806,10 +804,10 @@ public Literal visitTimeEscapedLiteral(TimeEscapedLiteralContext ctx) {
Source source = source(ctx);

// parse HH:mm:ss
DateTime dt = null;
LocalTime lt = null;
try {
dt = ISODateTimeFormat.hourMinuteSecond().parseDateTime(string);
} catch (IllegalArgumentException ex) {
lt = LocalTime.parse(string, ISO_LOCAL_TIME);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see ISO_LOCAL_TIME deals, also, with format as HH:mm (not only HH:mm:ss) and the previous joda implementation seems to have looked at HH:mm:ss only... not sure if this is an issue or not, meaning if there are 0 seconds, does it return 12:13 only or 12:13:00?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will dive into that when we implement the TIME data type, since now this method always throws an exception throw new SqlIllegalArgumentException("Time (only) literals are not supported; a date component is required as well");

} catch (DateTimeParseException ex) {
throw new ParsingException(source, "Invalid time received; {}", ex.getMessage());
}

Expand All @@ -822,18 +820,11 @@ public Literal visitTimestampEscapedLiteral(TimestampEscapedLiteralContext ctx)

Source source = source(ctx);
// parse yyyy-mm-dd hh:mm:ss(.f...)
DateTime dt = null;
try {
DateTimeFormatter formatter = new DateTimeFormatterBuilder()
.append(ISODateTimeFormat.date())
.appendLiteral(" ")
.append(ISODateTimeFormat.hourMinuteSecondFraction())
.toFormatter();
dt = formatter.parseDateTime(string);
} catch (IllegalArgumentException ex) {
return new Literal(source, ofEscapedLiteral(string), DataType.DATETIME);
} catch (DateTimeParseException ex) {
throw new ParsingException(source, "Invalid timestamp received; {}", ex.getMessage());
}
return new Literal(source, DateUtils.asDateTime(dt), DataType.DATETIME);
}

@Override
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.xpack.sql.util.DateUtils;

import java.time.ZonedDateTime;
import java.time.format.DateTimeParseException;
import java.util.Locale;
import java.util.function.DoubleFunction;
import java.util.function.Function;
Expand Down Expand Up @@ -546,8 +547,8 @@ private static Function<Object, Object> fromString(Function<String, Object> conv
return converter.apply(value.toString());
} catch (NumberFormatException e) {
throw new SqlIllegalArgumentException(e, "cannot cast [{}] to [{}]", value, to);
} catch (IllegalArgumentException e) {
throw new SqlIllegalArgumentException(e, "cannot cast [{}] to [{}]:{}", value, to, e.getMessage());
} catch (DateTimeParseException | IllegalArgumentException e) {
throw new SqlIllegalArgumentException(e, "cannot cast [{}] to [{}]: {}", value, to, e.getMessage());
}
};
}
Expand Down
61 changes: 20 additions & 41 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,35 @@

package org.elasticsearch.xpack.sql.util;

import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.time.DateFormatters;
import org.elasticsearch.xpack.sql.proto.StringUtils;
import org.joda.time.DateTime;
import org.joda.time.format.DateTimeFormatter;
import org.joda.time.format.ISODateTimeFormat;

import java.time.Instant;
import java.time.LocalDateTime;
import java.time.LocalDate;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;

import static java.time.format.DateTimeFormatter.ISO_LOCAL_DATE;
import static java.time.format.DateTimeFormatter.ISO_LOCAL_TIME;

public final class DateUtils {

private static final long DAY_IN_MILLIS = 60 * 60 * 24 * 1000;

// TODO: do we have a java.time based parser we can use instead?
private static final DateTimeFormatter UTC_DATE_FORMATTER = ISODateTimeFormat.dateOptionalTimeParser().withZoneUTC();
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be the same if we use DateFormatters.forPattern("date_optional_time") ?

Copy link
Member

Choose a reason for hiding this comment

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

It might work (see the TODO above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I use the date_optional_time and try to parse a date only String like 2019-02-06 I get:

java.time.DateTimeException: Unable to obtain Instant from TemporalAccessor: {},ISO resolved to 1970-01-01 of type java.time.format.Parsed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I need the same formatter but with:

.parseDefaulting(HOUR_OF_DAY, 0)
.parseDefaulting(MINUTE_OF_HOUR, 0)
.parseDefaulting(SECOND_OF_MINUTE, 0)


public static final ZoneId UTC = ZoneId.of("Z");
public static final String DATE_PARSE_FORMAT = "epoch_millis";

private static final DateTimeFormatter DATE_TIME_ESCAPED_LITERAL_FORMATTER = new DateTimeFormatterBuilder()
.append(ISO_LOCAL_DATE)
.appendLiteral(" ")
.append(ISO_LOCAL_TIME)
.toFormatter().withZone(UTC);

private static final DateFormatter UTC_DATE_TIME_FORMATTER = DateFormatter.forPattern("date_optional_time").withZone(UTC);

private static final long DAY_IN_MILLIS = 60 * 60 * 24 * 1000L;

private DateUtils() {}

/**
Expand Down Expand Up @@ -56,22 +62,7 @@ public static ZonedDateTime asDateTime(long millis, ZoneId id) {
* Parses the given string into a Date (SQL DATE type) using UTC as a default timezone.
*/
public static ZonedDateTime asDateOnly(String dateFormat) {
return asDateOnly(UTC_DATE_FORMATTER.parseDateTime(dateFormat));
}

public static ZonedDateTime asDateOnly(DateTime dateTime) {
LocalDateTime ldt = LocalDateTime.of(
dateTime.getYear(),
dateTime.getMonthOfYear(),
dateTime.getDayOfMonth(),
0,
0,
0,
0);

return ZonedDateTime.ofStrict(ldt,
ZoneOffset.ofTotalSeconds(dateTime.getZone().getOffset(dateTime) / 1000),
org.elasticsearch.common.time.DateUtils.dateTimeZoneToZoneId(dateTime.getZone()));
return LocalDate.parse(dateFormat, ISO_LOCAL_DATE).atStartOfDay(UTC);
}

public static ZonedDateTime asDateOnly(ZonedDateTime zdt) {
Expand All @@ -82,25 +73,13 @@ public static ZonedDateTime asDateOnly(ZonedDateTime zdt) {
* Parses the given string into a DateTime using UTC as a default timezone.
*/
public static ZonedDateTime asDateTime(String dateFormat) {
return asDateTime(UTC_DATE_FORMATTER.parseDateTime(dateFormat));
return DateFormatters.from(UTC_DATE_TIME_FORMATTER.parse(dateFormat)).withZoneSameInstant(UTC);
}

public static ZonedDateTime asDateTime(DateTime dateTime) {
LocalDateTime ldt = LocalDateTime.of(
dateTime.getYear(),
dateTime.getMonthOfYear(),
dateTime.getDayOfMonth(),
dateTime.getHourOfDay(),
dateTime.getMinuteOfHour(),
dateTime.getSecondOfMinute(),
dateTime.getMillisOfSecond() * 1_000_000);

return ZonedDateTime.ofStrict(ldt,
ZoneOffset.ofTotalSeconds(dateTime.getZone().getOffset(dateTime) / 1000),
org.elasticsearch.common.time.DateUtils.dateTimeZoneToZoneId(dateTime.getZone()));
public static ZonedDateTime ofEscapedLiteral(String dateFormat) {
return ZonedDateTime.parse(dateFormat, DATE_TIME_ESCAPED_LITERAL_FORMATTER.withZone(UTC));
}


public static String toString(ZonedDateTime dateTime) {
return StringUtils.toString(dateTime);
}
Expand Down
9 changes: 1 addition & 8 deletions ...va/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,15 @@
package org.elasticsearch.xpack.sql.expression.function.scalar.datetime;

import org.elasticsearch.xpack.sql.util.DateUtils;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;

import java.time.ZonedDateTime;

import static org.junit.Assert.assertEquals;

public class DateTimeTestUtils {

private DateTimeTestUtils() {}

public static ZonedDateTime dateTime(int year, int month, int day, int hour, int minute) {
DateTime dateTime = new DateTime(year, month, day, hour, minute, DateTimeZone.UTC);
ZonedDateTime zdt = ZonedDateTime.of(year, month, day, hour, minute, 0, 0, DateUtils.UTC);
assertEquals(dateTime.getMillis() / 1000, zdt.toEpochSecond());
return zdt;
return ZonedDateTime.of(year, month, day, hour, minute, 0, 0, DateUtils.UTC);
}

public static ZonedDateTime dateTime(long millisSinceEpoch) {
Expand Down
8 changes: 5 additions & 3 deletions ...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/EscapedFunctionsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ public void testDateLiteral() {

public void testDateLiteralValidation() {
ParsingException ex = expectThrows(ParsingException.class, () -> dateLiteral("2012-13-01"));
assertEquals("line 1:2: Invalid date received; Cannot parse \"2012-13-01\": Value 13 for monthOfYear must be in the range [1,12]",
assertEquals("line 1:2: Invalid date received; Text '2012-13-01' could not be parsed: " +
"Invalid value for MonthOfYear (valid values 1 - 12): 13",
ex.getMessage());
}

Expand All @@ -186,7 +187,8 @@ public void testTimeLiteralUnsupported() {

public void testTimeLiteralValidation() {
ParsingException ex = expectThrows(ParsingException.class, () -> timeLiteral("10:10:65"));
assertEquals("line 1:2: Invalid time received; Cannot parse \"10:10:65\": Value 65 for secondOfMinute must be in the range [0,59]",
assertEquals("line 1:2: Invalid time received; Text '10:10:65' could not be parsed: " +
"Invalid value for SecondOfMinute (valid values 0 - 59): 65",
ex.getMessage());
}

Expand All @@ -198,7 +200,7 @@ public void testTimestampLiteral() {
public void testTimestampLiteralValidation() {
ParsingException ex = expectThrows(ParsingException.class, () -> timestampLiteral("2012-01-01T10:01:02.3456"));
assertEquals(
"line 1:2: Invalid timestamp received; Invalid format: \"2012-01-01T10:01:02.3456\" is malformed at \"T10:01:02.3456\"",
"line 1:2: Invalid timestamp received; Text '2012-01-01T10:01:02.3456' could not be parsed at index 10",
ex.getMessage());
}

Expand Down
14 changes: 8 additions & 6 deletions ...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,18 +151,18 @@ public void testConversionToDate() {
Conversion conversion = conversionFor(KEYWORD, to);
assertNull(conversion.convert(null));

assertEquals(date(0L), conversion.convert("1970-01-01T00:10:01Z"));
assertEquals(date(1483228800000L), conversion.convert("2017-01-01T00:11:00Z"));
assertEquals(date(-1672531200000L), conversion.convert("1917-01-01T00:11:00Z"));
assertEquals(date(18000000L), conversion.convert("1970-01-01T03:10:20-05:00"));
assertEquals(date(0L), conversion.convert("1970-01-01"));
assertEquals(date(1483228800000L), conversion.convert("2017-01-01"));
assertEquals(date(-1672531200000L), conversion.convert("1917-01-01"));
assertEquals(date(18000000L), conversion.convert("1970-01-01"));

// double check back and forth conversion
ZonedDateTime zdt = TestUtils.now();
Conversion forward = conversionFor(DATE, KEYWORD);
Conversion back = conversionFor(KEYWORD, DATE);
assertEquals(DateUtils.asDateOnly(zdt), back.convert(forward.convert(zdt)));
Exception e = expectThrows(SqlIllegalArgumentException.class, () -> conversion.convert("0xff"));
assertEquals("cannot cast [0xff] to [date]:Invalid format: \"0xff\" is malformed at \"xff\"", e.getMessage());
assertEquals("cannot cast [0xff] to [date]: Text '0xff' could not be parsed at index 0", e.getMessage());
}
}

Expand Down Expand Up @@ -199,6 +199,7 @@ public void testConversionToDateTime() {
Conversion conversion = conversionFor(KEYWORD, to);
assertNull(conversion.convert(null));

assertEquals(dateTime(0L), conversion.convert("1970-01-01"));
assertEquals(dateTime(1000L), conversion.convert("1970-01-01T00:00:01Z"));
assertEquals(dateTime(1483228800000L), conversion.convert("2017-01-01T00:00:00Z"));
assertEquals(dateTime(1483228800000L), conversion.convert("2017-01-01T00:00:00Z"));
Expand All @@ -210,7 +211,8 @@ public void testConversionToDateTime() {
Conversion back = conversionFor(KEYWORD, DATETIME);
assertEquals(dt, back.convert(forward.convert(dt)));
Exception e = expectThrows(SqlIllegalArgumentException.class, () -> conversion.convert("0xff"));
assertEquals("cannot cast [0xff] to [datetime]:Invalid format: \"0xff\" is malformed at \"xff\"", e.getMessage());
assertEquals("cannot cast [0xff] to [datetime]: failed to parse date field [0xff] with format [date_optional_time]",
e.getMessage());
}
}

Expand Down