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.

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: Introduce SQL DATE data type #37693

Merged
merged 18 commits into from
Jan 24, 2019
Merged

SQL: Introduce SQL DATE data type #37693

merged 18 commits into from
Jan 24, 2019

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Jan 22, 2019

Support ANSI SQL's DATE type by introducing a runtime-only
ES SQL date type.

Closes: #37340

Support ANSI SQL's DATE type by introducing a runtime-only
ES SQL date type.

Closes: elastic#37340
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@matriv
Copy link
Contributor Author

matriv commented Jan 22, 2019

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

Overall it looks good however there are some things that need a bit of adjusting.

@@ -7,7 +7,10 @@ beta[]

Most of {es} <<mapping-types, data types>> are available in {es-sql}, as indicated below.
As one can see, all of {es} <<mapping-types, data types>> are mapped to the data type with the same
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be better emphasized - in asciidoc this can be achieve through admonitions (https://asciidoctor.org/docs/asciidoc-writers-guide/#admonitions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AS NOTE or IMPORTANT ?

Copy link
Member

Choose a reason for hiding this comment

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

I would pick NOTE since it's just FYI

@@ -33,6 +36,7 @@ s|SQL precision
| <<text, `text`>> | text | VARCHAR | 2,147,483,647
| <<binary, `binary`>> | binary | VARBINARY | 2,147,483,647
| <<date, `date`>> | datetime | TIMESTAMP | 24
| | date | DATE | 24
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the interval table below which contains the SQL -specific types not available in ES.

try {
zdt = ISO_WITH_MILLIS.parse(date, ZonedDateTime::from);
} catch (DateTimeParseException e) {
// Case of a Group By with casting as DATE
Copy link
Member

Choose a reason for hiding this comment

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

Instead of handling the exception, it's better to modify the formatter instead to make some parsing optional (optionalStart(), optionalEnd()).
I'm unclear though why would a GROUP BY cast on a DATE return an ISO date without seconds...

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 was wrong. Forgot to revisit this part. Check: 7409ce4

};
}
if (EsType.DATE == type) {
return JdbcDateUtils.asDate((String) val).getTime();
Copy link
Member

Choose a reason for hiding this comment

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

Since the Date object is not needed, simply open up utcMillisRemove method and use that instead to get the underlying long. Putting that in Date and then getting it out in the worse case scenario won't have any effect however its internal Calendar optimization might do.

@@ -0,0 +1,89 @@
//
// Date
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -170,12 +173,12 @@ public static TypeResolution typeMustBeString(Expression e, String operationName
return typeMustBe(e, DataType::isString, operationName, paramOrd, "string");
}

public static TypeResolution typeMustBeDate(Expression e, String operationName, ParamOrdinal paramOrd) {
return typeMustBe(e, dt -> dt == DataType.DATETIME, operationName, paramOrd, "date");
public static TypeResolution typeMustBeDateOrDateTime(Expression e, String operationName, ParamOrdinal paramOrd) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we pretty much accept both date and datetime, there's no reason to specify them in the method names - it just makes them longer.

// interval and dates
if (left == DATETIME) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be improved an currently allows more cases then it should.

With a date and interval, the date should always be on the left.
That's because date - interval works, but interval - date does not. It's something that I see most other DBs use as well.

Also please keep the current style which is simpler to read - that is only one type check per if.
if (left == DATE) { if (right == DATETIME) ... if (DataType.isInterval(...)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But:

postgres=# select '2019-01-01'::date + INTERVAL '1' MONTH;
      ?column?
---------------------
 2019-02-01 00:00:00
(1 row)

postgres=# select INTERVAL '1' MONTH + '2019-01-01'::date;
      ?column?
---------------------
 2019-02-01 00:00:00
(1 row)

Copy link
Contributor Author

@matriv matriv Jan 22, 2019

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It's the non-commutative operations that I'm concerned, in particular - (see BinaryArithmeticProcessor).
If the order doesn't matter, we need to check that - is not affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's not enough though - the case should be caught in the type resolution and reported accordingly. Currently the type resolution passes and an exception is thrown instead (which typically would occur if the plan passes but somehow the underlying data is not correct).

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 don't get this, if we want to support INTERVAL + date we need that.
The commonTypes() is not the place to check this imho, this should be checked specifically for the Subtrack operation for example.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not suggesting to fix this in commonType - instead that the type resolution of Sub should add an extra check.
Before this PR commonType did not allow the interval to be on the left and the date on the right and Sub was the reason; if commonType is changed then Sub needs to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okidoki, I misundertood you, sorry, adding the check and a test.

@@ -145,6 +161,8 @@ private static Conversion conversion(DataType from, DataType to) {
return conversionToFloat(from);
case DOUBLE:
return conversionToDouble(from);
case DATE:
Copy link
Member

Choose a reason for hiding this comment

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

Please be consistent - either DATE comes before DATETIME or vice-versa - in the Conversion check above it's the other way around.

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 have DATE before DATETIME, will check again for the consistency, thx.

return l -> ((ZonedDateTime) l).toEpochSecond();

private static Function<Object, Object> fromDateTime(Function<Long, Object> converter) {
return l -> converter.apply(((ZonedDateTime) l).toEpochSecond());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be epoch millis?

Copy link
Contributor Author

@matriv matriv Jan 22, 2019

Choose a reason for hiding this comment

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

I added a new issue #37655 and will have a separate PR because we want to backport this fix to earlier versions.

@@ -67,6 +69,9 @@ public static DataType fromJava(Object value) {
if (value instanceof Short) {
return SHORT;
}
if (value instanceof LocalDateTime) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we create a LocalDateTime instead of ZonedDateTime anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, that's a leftover, thx.

DAY_OF_WEEK(CAST(birth_date AS DATE)) dw,
DAY_OF_YEAR(CAST(birth_date AS DATE)) dy,
ISO_DAY_OF_WEEK(CAST(birth_date AS DATE)) iso_dw,
WEEK(CAST(birth_date AS DATE)) w,
Copy link
Contributor

Choose a reason for hiding this comment

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

You could, also, add the ISO_WEEK_OF_YEAR function, since you added the iso_dow one.

DAY_OF_YEAR(CAST(birth_date AS DATE)) dy,
ISO_DAY_OF_WEEK(CAST(birth_date AS DATE)) iso_dw,
WEEK(CAST(birth_date AS DATE)) w,
QUARTER(CAST(hire_date AS DATE)) q,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you used a different field... intentional or was just a small miss? (I was looking at the data returned by the query and the quarters didn't make any sense :-)), then I noticed this is a different field)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -378,7 +378,7 @@ public void testNotSupportedAggregateOnDate() {
}

public void testNotSupportedAggregateOnString() {
assertEquals("1:8: [MAX(keyword)] argument must be [numeric or date], found value [keyword] type [keyword]",
assertEquals("1:8: [MAX(keyword)] argument must be [numeric or datetime or date], found value [keyword] type [keyword]",
Copy link
Contributor

Choose a reason for hiding this comment

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

numeric, datetime or date seems a more natural type of enumeration in writing.

Copy link
Member

Choose a reason for hiding this comment

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

I would order the words in alphabetical order: date, datetime or numeric.


// The date return can differ due to the randomized timezones (it's transformed to the JDBC client's timezone)
dateAsGroupingKey-Ignore
SELECT CAST(birth_date AS DATE) AS d, CAST(SUM(emp_no) AS INT) s FROM test_emp GROUP BY d ORDER BY d LIMIT 5;
Copy link
Member

Choose a reason for hiding this comment

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

why not use a function than to extract just the month or the year?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe use the year. I don't understand why we would add a test which is disabled since the start.
Either there's a bug/missing feature so the test will be enabled in the future or the test is too random and as such, it should be rewritten or removed.

Copy link
Contributor Author

@matriv matriv Jan 22, 2019

Choose a reason for hiding this comment

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

No, there is not point, I will remove.

@matriv
Copy link
Contributor Author

matriv commented Jan 22, 2019

@costin @astefan
I have pushed fixups for the comments + a commit that enables the HISTOGRAMs on DATEs.

@@ -76,3 +76,8 @@ Instead one can rewrite the query to move the expression on the histogram _insid
----
include-tagged::{sql-specs}/docs.csv-spec[histogramDateTimeExpression]
----

[NOTE]
When the histogram in SQL is applied on **DATE** type instead of **DATETIME**, then the interval specified is rounded to
Copy link
Member

Choose a reason for hiding this comment

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

I would make this IMPORTANT.
Also drop the "then" : `when... DATETIME, the interval".


// The date return can differ due to the randomized timezones (it's transformed to the JDBC client's timezone)
dateAsGroupingKey-Ignore
SELECT CAST(birth_date AS DATE) AS d, CAST(SUM(emp_no) AS INT) s FROM test_emp GROUP BY d ORDER BY d LIMIT 5;
Copy link
Member

Choose a reason for hiding this comment

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

Then maybe use the year. I don't understand why we would add a test which is disabled since the start.
Either there's a bug/missing feature so the test will be enabled in the future or the test is too random and as such, it should be rewritten or removed.

@@ -95,7 +95,9 @@ public Object extract(Bucket bucket) {
if (object == null) {
return object;
} else if (object instanceof Long) {
object = DateUtils.of(((Long) object).longValue(), zoneId);
object = DateUtils.asDateTime(((Long) object).longValue(), zoneId);
} else if (object instanceof String) { // CAST(<value> AS DATE) is used
Copy link
Member

Choose a reason for hiding this comment

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

This looks suspicious - why would the key be a string and that would represent only a date and not a datetime?

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 zoneId != null we now we are expecting a DATETIME or DATE.
If it's a Long it's a DATETIME (ES date), If it's a DATE then there is a CAST applied with painless and the object extracted is a String. Do you have some other idea? Maybe I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a more detailed comment for the instanceof String branch?

Copy link
Member

Choose a reason for hiding this comment

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

The painless script is unnecessary and complicated namely because a histogram with a granularity of 1 day is going to ignore the time component anyway. We can and should identify this pattern GROUP BY - CAST(DATETIME->DATE) and simply convert that to a GROUP BY (DATETIME).
I'm not sure if the time will show-up or not (I would expect it to be zero anyway due to 1 day granularity).
Even if we were to use the painless script, since the casting is to a date, it should still return a ZonedDateTime and not a String however as I mention, we can optimize the script away.

I'm fine with addressing this in a different PR.

}
}
if (left == DATETIME) {
if (right == DATE || DataTypes.isInterval(right)) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure where my comment went but let's keep one condition per if - it's more readable this way.

Copy link
Member

Choose a reason for hiding this comment

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

Also the comment around the operation commutativity was removed (likely due to a force push - let's not do that in the future).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't force push, it's here: #37693 (comment)

forgot to fix the ifs, doing that.

}

public static long minDayInterval(long l) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we round a sub day to a day, the rounding if any should be ceiling not the floor.
However I think it should be a proper round meaning floor and ceiling.

Math.max(DAY_IN_MILLIS, l + (l % DAY_IN_MILLS << 2 >= DAY_IN_MILLIS ? 1 : 0))

Copy link
Contributor Author

@matriv matriv Jan 22, 2019

Choose a reason for hiding this comment

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

I don’t have a strong opinion for this. It was more or less a “gut feeling” to floor when > 1 day and somehow “related” (but it’s not really) to datetime -> date casting that truncates the time part and doesn’t round up.

@matriv
Copy link
Contributor Author

matriv commented Jan 22, 2019

@elasticmachine run elasticsearch-ci/default-distro

1 similar comment
@matriv
Copy link
Contributor Author

matriv commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/default-distro

@matriv
Copy link
Contributor Author

matriv commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/1

@matriv
Copy link
Contributor Author

matriv commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/default-distro

@matriv
Copy link
Contributor Author

matriv commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/1

@matriv
Copy link
Contributor Author

matriv commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/default-distro

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM. Left few final comments. Also, related to testing, I mentioned negative numbers to be used in tests for date/datetime conversions, but thinking about this some more, would make sense to have some randomization in these tests together with the fixed values we already have now?

@@ -95,7 +95,9 @@ public Object extract(Bucket bucket) {
if (object == null) {
return object;
} else if (object instanceof Long) {
object = DateUtils.of(((Long) object).longValue(), zoneId);
object = DateUtils.asDateTime(((Long) object).longValue(), zoneId);
} else if (object instanceof String) { // CAST(<value> AS DATE) is used
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a more detailed comment for the instanceof String branch?

@@ -45,6 +47,9 @@ protected TypeResolution resolveType() {
if (DataTypeConversion.commonType(l, r) == null) {
return new TypeResolution(format("[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r));
} else {
if (function() == SUB && right().dataType().isDateBased() && DataTypes.isInterval(left().dataType())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can do if (function() == SUB && r.isDateBased() && DataTypes.isInterval(l)) (r and l are already defined above).

Copy link
Member

Choose a reason for hiding this comment

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

The type check should happen inside SUB not in its parent class...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a mistake because of a mixup and a commit revert, sorry.......... :-(

long intervalAsMillis = Intervals.inMillis(h.interval());
if (h.dataType() == DATE) {
intervalAsMillis = DateUtils.minDayInterval(intervalAsMillis);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming these two lines here cover the scenario described in the documentation here https://github.com/elastic/elasticsearch/pull/37693/files#diff-fd0095853b8c10e4fd39bd7ae4958aabR80. Can you, please, add a small comment in code for this?

@@ -45,6 +45,7 @@
// the precision is 23 (number of chars in ISO8601 with millis) + Z (the UTC timezone)
// see https://github.com/elastic/elasticsearch/issues/30386#issuecomment-386807288
DATETIME( JDBCType.TIMESTAMP, Long.BYTES, 24, 24, false, false, true),
DATE( JDBCType.DATE, Long.BYTES, 10, 10, false, false, true),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for DATE being defined after DATETIME? In general, we list enums/constants/etc. in alphabetical order. In this same class, for example, DATE is always considered before DATETIME.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope will fix that.

@@ -157,6 +181,9 @@ private static Conversion conversion(DataType from, DataType to) {

private static Conversion conversionToString(DataType from) {
if (from == DATETIME) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here DATETIME comes before DATE and below in the same class is the other way around.

{
Conversion conversion = conversionFor(DATETIME, to);
assertNull(conversion.convert(null));
assertEquals("1973-11-29T21:33:09.101Z", conversion.convert(asDateTime(123456789101L)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for a negative long, as well? And not only in this test, but everywhere where numbers and dates are involved.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

The sub type resolution still needs addressing.


Obviously, not all types in {es} have an equivalent in SQL and vice-versa hence why, {es-sql}
uses the data type _particularities_ of the former over the latter as ultimately {es} is the backing store.

In addition to the types above, {es-sql} also supports at _runtime_ SQL-specific types that do not have an equivalent in {es}.
Such types cannot be loaded from {es} (as it does not know about them) however can be used inside {es-sql} in queries or their results.

[[es-sql-extra-types]]
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this to es-sql-only-types, extra doesn't really mean much.

@@ -95,7 +95,9 @@ public Object extract(Bucket bucket) {
if (object == null) {
return object;
} else if (object instanceof Long) {
object = DateUtils.of(((Long) object).longValue(), zoneId);
object = DateUtils.asDateTime(((Long) object).longValue(), zoneId);
} else if (object instanceof String) { // CAST(<value> AS DATE) is used
Copy link
Member

Choose a reason for hiding this comment

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

The painless script is unnecessary and complicated namely because a histogram with a granularity of 1 day is going to ignore the time component anyway. We can and should identify this pattern GROUP BY - CAST(DATETIME->DATE) and simply convert that to a GROUP BY (DATETIME).
I'm not sure if the time will show-up or not (I would expect it to be zero anyway due to 1 day granularity).
Even if we were to use the painless script, since the casting is to a date, it should still return a ZonedDateTime and not a String however as I mention, we can optimize the script away.

I'm fine with addressing this in a different PR.

* Creates an date for SQL DATE type from the millis since epoch.
*/
public static ZonedDateTime asDateOnly(long millis) {
return ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), UTC)
Copy link
Member

Choose a reason for hiding this comment

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

The with methods are useful when changing only one param - here however 3 intermediate objects are created.
Use ZonedDateTime.truncatedTo instead (recommended) or ZonedDateTime.of

}

public static ZonedDateTime asDateOnly(ZonedDateTime zdt) {
return zdt
Copy link
Member

Choose a reason for hiding this comment

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

Same as above - use truncatedTo or of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of and truncatedTo are in our forbiddenApis:

Forbidden method invocation: java.time.ZonedDateTime#of(int, int, int, int, int, int, int, java.time.ZoneId) [Local times may be ambiguous or nonexistent in a specific time zones. Use ZoneRules#getValidOffsets() instead.]
  in org.elasticsearch.xpack.sql.util.DateUtils (DateUtils.java:38)
Forbidden method invocation: java.time.ZonedDateTime#truncatedTo(java.time.temporal.TemporalUnit) [Local times may be ambiguous or nonexistent in a specific time zones. Use ZoneRules#getValidOffsets() instead.]
  in org.elasticsearch.xpack.sql.util.DateUtils (DateUtils.java:38)

public void testSubtractFromInterval() {
Analyzer analyzer = new Analyzer(TestUtils.TEST_CFG, new FunctionRegistry(), indexResolution, new Verifier(new Metrics()));
SqlIllegalArgumentException e = expectThrows(SqlIllegalArgumentException.class, () -> analyzer.analyze(parser.createStatement(
"SELECT INTERVAL 1 MONTH - CAST('2000-01-01' AS DATETIME)"), true));
Copy link
Member

Choose a reason for hiding this comment

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

This should throw an resolution exception not illegal argument exception (going back to Sub not properly validating the order of its args).

@matriv
Copy link
Contributor Author

matriv commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/2

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM though there are still 2 new comments that need addressing.

@@ -45,6 +47,9 @@ protected TypeResolution resolveType() {
if (DataTypeConversion.commonType(l, r) == null) {
return new TypeResolution(format("[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r));
} else {
if (function() == SUB && right().dataType().isDateBased() && DataTypes.isInterval(left().dataType())) {
Copy link
Member

Choose a reason for hiding this comment

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

The type check should happen inside SUB not in its parent class...

}

public static ZonedDateTime asDateOnly(ZonedDateTime zdt) {
return zdt.toLocalDate().atStartOfDay(UTC);
Copy link
Member

Choose a reason for hiding this comment

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

The ZoneId should be that of the received zdt not UTC...

@astefan
Copy link
Contributor

astefan commented Jan 24, 2019

run the gradle build tests 2

@astefan
Copy link
Contributor

astefan commented Jan 24, 2019

@elasticmachine run elasticsearch-ci/2

@matriv
Copy link
Contributor Author

matriv commented Jan 24, 2019

@elasticmachine run elasticsearch-ci/1

@matriv
Copy link
Contributor Author

matriv commented Jan 24, 2019

@costin @astefan Thank you very much for the repeated reviews, your comments, suggestions and overall your patience with this PR.

@matriv matriv merged commit f707fa9 into elastic:master Jan 24, 2019
@matriv matriv deleted the mt/impl-37340 branch January 24, 2019 11:42
@matriv
Copy link
Contributor Author

matriv commented Jan 24, 2019

Backported to 6.x with d149d8f

matriv added a commit that referenced this pull request Jan 24, 2019
Support ANSI SQL's DATE type by introducing a runtime-only
ES SQL date type.

Closes: #37340
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 24, 2019
* elastic/master:
  Optimize warning header de-duplication (elastic#37725)
  Bubble exceptions up in ClusterApplierService (elastic#37729)
  SQL: Improve handling of invalid args for PERCENTILE/PERCENTILE_RANK (elastic#37803)
  Remove unused ThreadBarrier class (elastic#37666)
  Add built-in user and role for code plugin (elastic#37030)
  Consolidate testclusters tests into a single project (elastic#37362)
  Fix docs for MappingUpdatedAction
  SQL: Introduce SQL DATE data type (elastic#37693)
  disabling bwc test while backporting elastic#37639
  Mute ClusterDisruptionIT testAckedIndexing
  Set acking timeout to 0 on dynamic mapping update (elastic#31140)
  Remove index audit output type (elastic#37707)
  Mute FollowerFailOverIT testReadRequestsReturnsLatestMappingVersion
  [ML] Increase close job timeout and lower the max number (elastic#37770)
  Remove Custom Listeners from SnapshotsService (elastic#37629)
  Use m_m_nodes from Zen1 master for Zen2 bootstrap (elastic#37701)
  Fix index filtering in follow info api. (elastic#37752)
  Use project dependency instead of substitutions for distributions (elastic#37730)
  Update authenticate to allow unknown fields (elastic#37713)
  Deprecate HLRC EmptyResponse used by security (elastic#37540)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants