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
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
6 changes: 5 additions & 1 deletion docs/reference/sql/language/data-types.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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

name in {es-sql}, with the exception of **date** data type which is mapped to **datetime** in {es-sql}:
name in {es-sql}, with the exception of **date** data type which is mapped to **datetime** in {es-sql}.
This is to avoid confusion with the ANSI SQL date (date only) type, which is also supported by {es-sql}
in queries (with the use of <<sql-functions-type-conversion-cast>>/<<sql-functions-type-conversion-convert>>),
but doesn't correspond to an actual mapping in {es}.

[cols="^,^m,^,^"]

Expand All @@ -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.

| <<ip, `ip`>> | ip | VARCHAR | 39

4+h| Complex types
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public enum EsType implements SQLType {
OBJECT( Types.STRUCT),
NESTED( Types.STRUCT),
BINARY( Types.VARBINARY),
DATE( Types.DATE),
DATETIME( Types.TIMESTAMP),
IP( Types.VARCHAR),
INTERVAL_YEAR( ExtraTypes.INTERVAL_YEAR),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
import java.time.format.DateTimeParseException;
import java.util.Locale;
import java.util.function.Function;

Expand Down Expand Up @@ -41,9 +42,25 @@ final class JdbcDateUtils {
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendOffsetId()
.toFormatter(Locale.ROOT);

static final DateTimeFormatter ISO_WITH_MINUTES = new DateTimeFormatterBuilder()
.parseCaseInsensitive()
.append(ISO_LOCAL_DATE)
.appendLiteral('T')
.appendValue(HOUR_OF_DAY, 2)
.appendLiteral(':')
.appendValue(MINUTE_OF_HOUR, 2)
.appendOffsetId()
.toFormatter(Locale.ROOT);

static long asMillisSinceEpoch(String date) {
ZonedDateTime zdt = ISO_WITH_MILLIS.parse(date, ZonedDateTime::from);
ZonedDateTime zdt;
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

zdt = ISO_WITH_MINUTES.parse(date, ZonedDateTime::from);
}
return zdt.toInstant().toEpochMilli();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,10 @@ private Long dateTime(int columnIndex) throws SQLException {
return null;
}
return JdbcDateUtils.asDateTimeField(val, JdbcDateUtils::asMillisSinceEpoch, Function.identity());
};
}
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.

}
return val == null ? null : (Long) val;
} catch (ClassCastException cce) {
throw new SQLException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ static Object convert(Object v, EsType columnType, String typeString) throws SQL
return doubleValue(v); // Double might be represented as string for infinity and NaN values
case FLOAT:
return floatValue(v); // Float might be represented as string for infinity and NaN values
case DATE:
return JdbcDateUtils.asDate(v.toString());
case DATETIME:
return JdbcDateUtils.asDateTimeField(v, JdbcDateUtils::asTimestamp, Timestamp::new);
case INTERVAL_YEAR:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public static List<Object[]> readScriptSpec() throws Exception {
tests.addAll(readScriptSpec("/fulltext.csv-spec", parser));
tests.addAll(readScriptSpec("/agg.csv-spec", parser));
tests.addAll(readScriptSpec("/columns.csv-spec", parser));
tests.addAll(readScriptSpec("/date.csv-spec", parser));
tests.addAll(readScriptSpec("/datetime.csv-spec", parser));
tests.addAll(readScriptSpec("/alias.csv-spec", parser));
tests.addAll(readScriptSpec("/null.csv-spec", parser));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ public static void assertResultSetMetadata(ResultSet expected, ResultSet actual,
if (expectedType == Types.TIMESTAMP_WITH_TIMEZONE) {
expectedType = Types.TIMESTAMP;
}

// since csv doesn't support real, we use float instead.....
if (expectedType == Types.FLOAT && expected instanceof CsvResultSet) {
expectedType = Types.REAL;
Expand Down Expand Up @@ -204,6 +205,9 @@ private static void doAssertResultSetData(ResultSet expected, ResultSet actual,
// fix for CSV which returns the shortName not fully-qualified name
if (!columnClassName.contains(".")) {
switch (columnClassName) {
case "Date":
columnClassName = "java.sql.Date";
break;
case "Timestamp":
columnClassName = "java.sql.Timestamp";
break;
Expand Down
90 changes: 90 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/date.csv-spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
//
// Date
Copy link
Member

Choose a reason for hiding this comment

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

👍

//

dateExtractDateParts
SELECT
DAY(CAST(birth_date AS DATE)) d,
DAY_OF_MONTH(CAST(birth_date AS DATE)) dm,
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.

IW(CAST(birth_date AS DATE)) iso_w,
QUARTER(CAST(birth_date AS DATE)) q,
YEAR(CAST(birth_date AS DATE)) y,
birth_date, last_name l FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no;

d:i | dm:i | dw:i | dy:i | iso_dw:i | w:i |iso_w:i | q:i | y:i | birth_date:ts | l:s
2 |2 |4 |245 |3 |36 |35 |3 |1953 |1953-09-02T00:00:00Z |Facello
2 |2 |3 |154 |2 |23 |22 |2 |1964 |1964-06-02T00:00:00Z |Simmel
3 |3 |5 |337 |4 |49 |49 |4 |1959 |1959-12-03T00:00:00Z |Bamford
1 |1 |7 |121 |6 |18 |18 |2 |1954 |1954-05-01T00:00:00Z |Koblick
21 |21 |6 |21 |5 |4 |3 |1 |1955 |1955-01-21T00:00:00Z |Maliniak
20 |20 |2 |110 |1 |17 |16 |2 |1953 |1953-04-20T00:00:00Z |Preusig
23 |23 |5 |143 |4 |21 |21 |2 |1957 |1957-05-23T00:00:00Z |Zielinski
19 |19 |4 |50 |3 |8 |8 |1 |1958 |1958-02-19T00:00:00Z |Kalloufi
19 |19 |7 |110 |6 |16 |16 |2 |1952 |1952-04-19T00:00:00Z |Peac
;


dateExtractTimePartsTimeSecond
SELECT
SECOND(CAST(birth_date AS DATE)) d,
MINUTE(CAST(birth_date AS DATE)) m,
HOUR(CAST(birth_date AS DATE)) h
FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no;

d:i | m:i | h:i
0 |0 |0
0 |0 |0
0 |0 |0
0 |0 |0
0 |0 |0
0 |0 |0
0 |0 |0
0 |0 |0
0 |0 |0
;

dateAsFilter
SELECT birth_date, last_name FROM "test_emp" WHERE birth_date <= CAST('1955-01-21' AS DATE) ORDER BY emp_no LIMIT 5;

birth_date:ts | last_name:s
1953-09-02T00:00:00Z |Facello
1954-05-01T00:00:00Z |Koblick
1955-01-21T00:00:00Z |Maliniak
1953-04-20T00:00:00Z |Preusig
1952-04-19T00:00:00Z |Peac
;


// 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.


d:date | s:i
null |100445
1952-02-26 |10097
1952-04-18 |10009
1952-05-14 |10072
1952-06-12 |10076
;

dateAndFunctionAsGroupingKey
SELECT MONTH(CAST(birth_date AS DATE)) AS m, CAST(SUM(emp_no) AS INT) s FROM test_emp GROUP BY m ORDER BY m LIMIT 5;

m:i | s:i
null |100445
1 |60288
2 |80388
3 |20164
4 |80401
;

dateAndInterval
SELECT YEAR(CAST('2019-01-21' AS DATE) + INTERVAL '1-2' YEAR TO MONTH) AS y, MONTH(INTERVAL '1-2' YEAR TO MONTH + CAST('2019-01-21' AS DATE)) AS m;

y:i | m:i
2020 | 3
;
9 changes: 5 additions & 4 deletions x-pack/plugin/sql/qa/src/main/resources/datetime.sql-spec
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
// Time NOT IMPLEMENTED in H2 on TIMESTAMP WITH TIME ZONE - hence why these are moved to CSV
//

// WEEK_OF_YEAR moved to CSV tests, because H2 builds its Calendar with the local Locale, we consider ROOT as the default Locale
// This has implications on the results, which could change given specific locales where the rules for determining the start of a year are different.
// WEEK_OF_YEAR moved to CSV tests, because H2 builds its Calendar with the local Locale,
// we consider ROOT as the default Locale. This has implications on the results, which could
// change given specific locales where the rules for determining the start of a year are different.

//
// DateTime
Expand All @@ -31,10 +32,10 @@ SELECT MONTHNAME(CAST('2018-09-03' AS TIMESTAMP)) month FROM "test_emp" limit 1;
dayNameFromStringDateTime
SELECT DAYNAME(CAST('2018-09-03' AS TIMESTAMP)) day FROM "test_emp" limit 1;

quarterSelect
dateTimeQuarter
Copy link
Member

Choose a reason for hiding this comment

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

the file name is already used as a prefix and adding dateTime in the test name becomes redundant:
{datetime.dateTimeQuarter}.

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.

I see - the others are inconsistent though :)

SELECT QUARTER(hire_date) q, hire_date FROM test_emp ORDER BY hire_date LIMIT 15;

dayOfWeek
dateTimeDayOfWeek
SELECT DAY_OF_WEEK(birth_date) day, birth_date FROM test_emp ORDER BY DAY_OF_WEEK(birth_date);

//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ 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 {
throw new SqlIllegalArgumentException("Invalid date key returned: {}", object);
}
Expand Down Expand Up @@ -129,4 +129,4 @@ public boolean equals(Object obj) {
public String toString() {
return "|" + key + "|";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,11 @@ private Object unwrapMultiValue(Object values) {
}
if (dataType == DataType.DATETIME) {
if (values instanceof String) {
return DateUtils.of(Long.parseLong(values.toString()));
return DateUtils.asDateTime(Long.parseLong(values.toString()));
}
// returned by nested types...
if (values instanceof DateTime) {
return DateUtils.of((DateTime) values);
return DateUtils.asDateTime((DateTime) values);
}
}
if (values instanceof Long || values instanceof Double || values instanceof String || values instanceof Boolean) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/
package org.elasticsearch.xpack.sql.expression;

import org.elasticsearch.common.Strings;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.expression.Expression.TypeResolution;
import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe;
Expand All @@ -16,11 +15,15 @@
import java.util.Collection;
import java.util.List;
import java.util.Locale;
import java.util.StringJoiner;
import java.util.function.Predicate;

import static java.lang.String.format;
import static java.util.Collections.emptyList;
import static java.util.Collections.emptyMap;
import static org.elasticsearch.xpack.sql.type.DataType.BOOLEAN;
import static org.elasticsearch.xpack.sql.type.DataType.DATE;
import static org.elasticsearch.xpack.sql.type.DataType.DATETIME;

public final class Expressions {

Expand Down Expand Up @@ -155,7 +158,7 @@ public static List<Pipe> pipe(List<Expression> expressions) {
}

public static TypeResolution typeMustBeBoolean(Expression e, String operationName, ParamOrdinal paramOrd) {
return typeMustBe(e, dt -> dt == DataType.BOOLEAN, operationName, paramOrd, "boolean");
return typeMustBe(e, dt -> dt == BOOLEAN, operationName, paramOrd, "boolean");
}

public static TypeResolution typeMustBeInteger(Expression e, String operationName, ParamOrdinal paramOrd) {
Expand All @@ -171,11 +174,11 @@ public static TypeResolution typeMustBeString(Expression e, String operationName
}

public static TypeResolution typeMustBeDate(Expression e, String operationName, ParamOrdinal paramOrd) {
return typeMustBe(e, dt -> dt == DataType.DATETIME, operationName, paramOrd, "date");
return typeMustBe(e, dt -> dt == DATETIME || dt == DATE, operationName, paramOrd, "datetime, date");
}

public static TypeResolution typeMustBeNumericOrDate(Expression e, String operationName, ParamOrdinal paramOrd) {
return typeMustBe(e, dt -> dt.isNumeric() || dt == DataType.DATETIME, operationName, paramOrd, "numeric", "date");
return typeMustBe(e, dt -> dt.isNumeric() || dt == DATETIME, operationName, paramOrd, "numeric", "datetime", "date");
}

public static TypeResolution typeMustBe(Expression e,
Expand All @@ -188,8 +191,20 @@ public static TypeResolution typeMustBe(Expression e,
new TypeResolution(format(Locale.ROOT, "[%s]%s argument must be [%s], found value [%s] type [%s]",
operationName,
paramOrd == null || paramOrd == ParamOrdinal.DEFAULT ? "" : " " + paramOrd.name().toLowerCase(Locale.ROOT),
Strings.arrayToDelimitedString(acceptedTypes, " or "),
acceptedTypesForErrorMsg(acceptedTypes),
Expressions.name(e),
e.dataType().esType));
}

private static String acceptedTypesForErrorMsg(String... acceptedTypes) {
StringJoiner sj = new StringJoiner(", ");
for (int i = 0; i < acceptedTypes.length - 1; i++) {
sj.add(acceptedTypes[i]);
}
if (acceptedTypes.length > 1) {
return sj.toString() + " or " + acceptedTypes[acceptedTypes.length - 1];
} else {
return acceptedTypes[0];
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,4 @@ public String innerName() {
protected TypeResolution resolveType() {
return Expressions.typeMustBeNumericOrDate(field(), sourceText(), ParamOrdinal.DEFAULT);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,4 @@ public boolean equals(Object obj) {
public int hashCode() {
return Objects.hash(field(), zoneId());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ private static Object asDateTime(Object dateTime, boolean lenient) {
}
if (false == lenient) {
if (dateTime instanceof Number) {
return DateUtils.of(((Number) dateTime).longValue());
return DateUtils.asDateTime(((Number) dateTime).longValue());
}

throw new SqlIllegalArgumentException("Invalid date encountered [{}]", dateTime);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,8 @@ public DataType visitPrimitiveDataType(PrimitiveDataTypeContext ctx) {
case "float":
case "double":
return DataType.DOUBLE;
case "date":
return DataType.DATE;
case "datetime":
case "timestamp":
return DataType.DATETIME;
Expand Down Expand Up @@ -793,7 +795,7 @@ public Literal visitDateEscapedLiteral(DateEscapedLiteralContext ctx) {
} catch(IllegalArgumentException ex) {
throw new ParsingException(source, "Invalid date received; {}", ex.getMessage());
}
return new Literal(source, DateUtils.of(dt), DataType.DATETIME);
return new Literal(source, DateUtils.asDateOnly(dt), DataType.DATE);
}

@Override
Expand Down Expand Up @@ -829,7 +831,7 @@ public Literal visitTimestampEscapedLiteral(TimestampEscapedLiteralContext ctx)
} catch (IllegalArgumentException ex) {
throw new ParsingException(source, "Invalid timestamp received; {}", ex.getMessage());
}
return new Literal(source, DateUtils.of(dt), DataType.DATETIME);
return new Literal(source, DateUtils.asDateTime(dt), DataType.DATETIME);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public enum DataType {
// 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.

//
// specialized types
//
Expand Down Expand Up @@ -102,7 +103,7 @@ public enum DataType {
odbcToEs.put("SQL_LONGVARBINARY", BINARY);

// Date
odbcToEs.put("SQL_DATE", DATETIME);
odbcToEs.put("SQL_DATE", DATE);
odbcToEs.put("SQL_TIME", DATETIME);
odbcToEs.put("SQL_TIMESTAMP", DATETIME);

Expand Down
Loading