-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Changes from 1 commit
95a0990
b410903
30f4fde
7409ce4
3be8945
7b9656f
5a2c05c
3ea4bdf
ffdfc3f
6bcc0af
89ed0ba
e7e880c
471187d
dc339f1
8dc2c4d
2341144
e4e6507
14d4986
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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,^,^"] | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the |
||
} | ||
return val == null ? null : (Long) val; | ||
} catch (ClassCastException cce) { | ||
throw new SQLException( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
// | ||
// Date | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
QUARTER(CAST(hire_date AS DATE)) q, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it was copied from here: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/qa/src/main/resources/datetime.sql-spec#L35 |
||
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 | q:i | y:i | birth_date:ts | l:s | ||
2 |2 |4 |245 |3 |36 |2 |1953 |1953-09-02T00:00:00Z |Facello | ||
2 |2 |3 |154 |2 |23 |4 |1964 |1964-06-02T00:00:00Z |Simmel | ||
3 |3 |5 |337 |4 |49 |3 |1959 |1959-12-03T00:00:00Z |Bamford | ||
1 |1 |7 |121 |6 |18 |4 |1954 |1954-05-01T00:00:00Z |Koblick | ||
21 |21 |6 |21 |5 |4 |3 |1955 |1955-01-21T00:00:00Z |Maliniak | ||
20 |20 |2 |110 |1 |17 |2 |1953 |1953-04-20T00:00:00Z |Preusig | ||
23 |23 |5 |143 |4 |21 |1 |1957 |1957-05-23T00:00:00Z |Zielinski | ||
19 |19 |4 |50 |3 |8 |3 |1958 |1958-02-19T00:00:00Z |Kalloufi | ||
19 |19 |7 |110 |6 |16 |1 |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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is here already: https://github.com/elastic/elasticsearch/pull/37693/files/b4109031e770ac52153184b32baf921c112e8acd#diff-51f788d55d8ade70340fdf2ca8af1f20R74 I used this query to discover this issue: #37693 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(CAST('2019-01-21' AS DATE) + INTERVAL '1-2' YEAR TO MONTH) AS m; | ||
|
||
y:i | m:i | ||
2020 | 3 | ||
; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, but currently I just tried to make ti consistent with all the others: https://github.com/elastic/elasticsearch/pull/37693/files/95a0990cfcc48c1aee6fdb0ab1ff9cb569aafd3a#diff-6079120a3b84ce0d0410436f58ec8a5bR17 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,9 @@ | |
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 { | ||
|
||
|
@@ -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) { | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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"); | ||
public static TypeResolution typeMustBeNumericOrDateTimeOrDate(Expression e, String operationName, ParamOrdinal paramOrd) { | ||
return typeMustBe(e, dt -> dt.isNumeric() || dt == DATETIME, operationName, paramOrd, "numeric", "datetime", "date"); | ||
} | ||
|
||
public static TypeResolution typeMustBe(Expression e, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,11 @@ public Literal(Source source, Object value, DataType dataType) { | |
public Literal(Source source, String name, Object value, DataType dataType) { | ||
super(source, name == null ? source.text() : name, emptyList(), null); | ||
this.dataType = dataType; | ||
this.value = DataTypeConversion.convert(value, dataType); | ||
// if (dataType == DATE) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, :-( this is again a left over from an attempt to use LocalDateTime for the |
||
// this.value = value; | ||
// } else { | ||
this.value = DataTypeConversion.convert(value, dataType); | ||
// } | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope will fix that. |
||
// | ||
// specialized types | ||
// | ||
|
@@ -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); | ||
|
||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AS
NOTE
orIMPORTANT
?There was a problem hiding this comment.
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