Skip to content

Commit

Permalink
Address comments pt1
Browse files Browse the repository at this point in the history
  • Loading branch information
@matriv
matriv committed Jan 22, 2019
1 parent 95a0990 commit b410903
Show file tree
Hide file tree
Showing 14 changed files with 61 additions and 58 deletions.
25 changes: 13 additions & 12 deletions x-pack/plugin/sql/qa/src/main/resources/date.csv-spec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,21 @@ 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,
QUARTER(CAST(hire_date AS DATE)) q,
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 | 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
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
;


Expand Down Expand Up @@ -82,7 +83,7 @@ null |100445
;

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;
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
Expand Down
20 changes: 16 additions & 4 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java
View file
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,6 +15,7 @@
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;
Expand Down Expand Up @@ -173,11 +173,11 @@ public static TypeResolution typeMustBeString(Expression e, String operationName
return typeMustBe(e, DataType::isString, operationName, paramOrd, "string");
}

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

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

Expand All @@ -191,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];
}
}
}
6 changes: 1 addition & 5 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,7 @@ 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;
// if (dataType == DATE) {
// this.value = value;
// } else {
this.value = DataTypeConversion.convert(value, dataType);
// }
this.value = DataTypeConversion.convert(value, dataType);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...ugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Max.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,6 @@ public String innerName() {

@Override
protected TypeResolution resolveType() {
return Expressions.typeMustBeNumericOrDateTimeOrDate(field(), sourceText(), ParamOrdinal.DEFAULT);
return Expressions.typeMustBeNumericOrDate(field(), sourceText(), ParamOrdinal.DEFAULT);
}
}
2 changes: 1 addition & 1 deletion ...ugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Min.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ public String innerName() {

@Override
protected TypeResolution resolveType() {
return Expressions.typeMustBeNumericOrDateTimeOrDate(field(), sourceText(), ParamOrdinal.DEFAULT);
return Expressions.typeMustBeNumericOrDate(field(), sourceText(), ParamOrdinal.DEFAULT);
}
}
2 changes: 1 addition & 1 deletion ...sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public ZoneId zoneId() {

@Override
protected TypeResolution resolveType() {
TypeResolution resolution = Expressions.typeMustBeNumericOrDateTimeOrDate(field(), "HISTOGRAM", ParamOrdinal.FIRST);
TypeResolution resolution = Expressions.typeMustBeNumericOrDate(field(), "HISTOGRAM", ParamOrdinal.FIRST);
if (resolution == TypeResolution.TYPE_RESOLVED) {
// interval must be Literal interval
if (field().dataType() == DataType.DATETIME) {
Expand Down
2 changes: 1 addition & 1 deletion ...org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ protected final NodeInfo<BaseDateTimeFunction> info() {

@Override
protected TypeResolution resolveType() {
return Expressions.typeMustBeDateOrDateTime(field(), sourceText(), ParamOrdinal.DEFAULT);
return Expressions.typeMustBeDate(field(), sourceText(), ParamOrdinal.DEFAULT);
}

public ZoneId zoneId() {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -795,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.asDate(dt), DataType.DATE);
return new Literal(source, DateUtils.asDateOnly(dt), DataType.DATE);
}

@Override
Expand Down
22 changes: 11 additions & 11 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,23 +76,23 @@ public static DataType commonType(DataType left, DataType right) {
}

// interval and dates
if (left == DATETIME) {
if (right == DATE || DataTypes.isInterval(right)) {
if (left == DATE) {
if (DataTypes.isInterval(right)) {
return left;
}
}
if (right == DATETIME) {
if (left == DATE || DataTypes.isInterval(left)) {
if (right == DATE) {
if (DataTypes.isInterval(left)) {
return right;
}
}
if (left == DATE) {
if (DataTypes.isInterval(right)) {
if (left == DATETIME) {
if (right == DATE || DataTypes.isInterval(right)) {
return left;
}
}
if (right == DATE) {
if (DataTypes.isInterval(left)) {
if (right == DATETIME) {
if (left == DATE || DataTypes.isInterval(left)) {
return right;
}
}
Expand Down Expand Up @@ -497,7 +497,7 @@ public enum Conversion {
RATIONAL_TO_DATE(toDate(RATIONAL_TO_LONG)),
INTEGER_TO_DATE(toDate(INTEGER_TO_LONG)),
BOOL_TO_DATE(toDate(BOOL_TO_INT)),
STRING_TO_DATE(fromString(DateUtils::asDate, "date")),
STRING_TO_DATE(fromString(DateUtils::asDateOnly, "date")),
DATETIME_TO_DATE(fromDatetimeToDate()),

RATIONAL_TO_DATETIME(toDateTime(RATIONAL_TO_LONG)),
Expand Down Expand Up @@ -559,11 +559,11 @@ private static Function<Object, Object> toDateTime(Conversion conversion) {
}

private static Function<Object, Object> toDate(Conversion conversion) {
return l -> DateUtils.asDate(((Number) conversion.convert(l)).longValue());
return l -> DateUtils.asDateOnly(((Number) conversion.convert(l)).longValue());
}

private static Function<Object, Object> fromDatetimeToDate() {
return l -> DateUtils.asDate((ZonedDateTime) l);
return l -> DateUtils.asDateOnly((ZonedDateTime) l);
}

public Object convert(Object l) {
Expand Down
5 changes: 0 additions & 5 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.expression.literal.Interval;

import java.time.LocalDateTime;
import java.time.ZonedDateTime;

import static org.elasticsearch.xpack.sql.type.DataType.BOOLEAN;
import static org.elasticsearch.xpack.sql.type.DataType.BYTE;
import static org.elasticsearch.xpack.sql.type.DataType.DATE;
import static org.elasticsearch.xpack.sql.type.DataType.DATETIME;
import static org.elasticsearch.xpack.sql.type.DataType.DOUBLE;
import static org.elasticsearch.xpack.sql.type.DataType.FLOAT;
Expand Down Expand Up @@ -69,9 +67,6 @@ public static DataType fromJava(Object value) {
if (value instanceof Short) {
return SHORT;
}
if (value instanceof LocalDateTime) {
return DATE;
}
if (value instanceof ZonedDateTime) {
return DATETIME;
}
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private DateUtils() {}
/**
* Creates an date for SQL DATE type from the millis since epoch.
*/
public static ZonedDateTime asDate(long millis) {
public static ZonedDateTime asDateOnly(long millis) {
return ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), UTC)
.withHour(0)
.withMinute(0)
Expand All @@ -63,11 +63,11 @@ 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 asDate(String dateFormat) {
return asDate(UTC_DATE_FORMATTER.parseDateTime(dateFormat));
public static ZonedDateTime asDateOnly(String dateFormat) {
return asDateOnly(UTC_DATE_FORMATTER.parseDateTime(dateFormat));
}

public static ZonedDateTime asDate(DateTime dateTime) {
public static ZonedDateTime asDateOnly(DateTime dateTime) {
LocalDateTime ldt = LocalDateTime.of(
dateTime.getYear(),
dateTime.getMonthOfYear(),
Expand All @@ -82,7 +82,7 @@ public static ZonedDateTime asDate(DateTime dateTime) {
org.elasticsearch.common.time.DateUtils.dateTimeZoneToZoneId(dateTime.getZone()));
}

public static ZonedDateTime asDate(ZonedDateTime zdt) {
public static ZonedDateTime asDateOnly(ZonedDateTime zdt) {
return zdt
.withHour(0)
.withMinute(0)
Expand Down
2 changes: 1 addition & 1 deletion ...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ public void testNotSupportedAggregateOnDate() {
}

public void testNotSupportedAggregateOnString() {
assertEquals("1:8: [MAX(keyword)] argument must be [numeric or datetime or date], found value [keyword] type [keyword]",
assertEquals("1:8: [MAX(keyword)] argument must be [numeric, datetime or date], found value [keyword] type [keyword]",
error("SELECT MAX(keyword) FROM test"));
}

Expand Down
2 changes: 1 addition & 1 deletion ...va/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeTestUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ public static ZonedDateTime dateTime(long millisSinceEpoch) {
}

public static ZonedDateTime date(long millisSinceEpoch) {
return DateUtils.asDate(millisSinceEpoch);
return DateUtils.asDateOnly(millisSinceEpoch);
}
}
17 changes: 8 additions & 9 deletions ...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import static org.elasticsearch.xpack.sql.type.DataType.values;
import static org.elasticsearch.xpack.sql.type.DataTypeConversion.commonType;
import static org.elasticsearch.xpack.sql.type.DataTypeConversion.conversionFor;
import static org.elasticsearch.xpack.sql.util.DateUtils.asDate;
import static org.elasticsearch.xpack.sql.util.DateUtils.asDateTime;


Expand All @@ -60,7 +59,7 @@ public void testConversionToString() {
{
Conversion conversion = conversionFor(DATE, to);
assertNull(conversion.convert(null));
assertEquals("1973-11-29", conversion.convert(asDate(123456789101L)));
assertEquals("1973-11-29", conversion.convert(DateUtils.asDateOnly(123456789101L)));
}
{
Conversion conversion = conversionFor(DATETIME, to);
Expand Down Expand Up @@ -98,7 +97,7 @@ public void testConversionToLong() {
{
Conversion conversion = conversionFor(DATE, to);
assertNull(conversion.convert(null));
assertEquals(123379200L, conversion.convert(asDate(123456789101L)));
assertEquals(123379200L, conversion.convert(DateUtils.asDateOnly(123456789101L)));
}
{
Conversion conversion = conversionFor(DATETIME, to);
Expand Down Expand Up @@ -155,7 +154,7 @@ public void testConversionToDate() {
ZonedDateTime zdt = TestUtils.now();
Conversion forward = conversionFor(DATE, KEYWORD);
Conversion back = conversionFor(KEYWORD, DATE);
assertEquals(DateUtils.asDate(zdt), back.convert(forward.convert(zdt)));
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());
}
Expand Down Expand Up @@ -187,7 +186,7 @@ public void testConversionToDateTime() {
{
Conversion conversion = conversionFor(DATE, to);
assertNull(conversion.convert(null));
assertEquals(dateTime(123379200000L), conversion.convert(asDate(123456789101L)));
assertEquals(dateTime(123379200000L), conversion.convert(DateUtils.asDateOnly(123456789101L)));
}
{
Conversion conversion = conversionFor(KEYWORD, to);
Expand Down Expand Up @@ -231,7 +230,7 @@ public void testConversionToDouble() {
{
Conversion conversion = conversionFor(DATE, to);
assertNull(conversion.convert(null));
assertEquals(1.233792E8, (double) conversion.convert(asDate(123456789101L)), 0);
assertEquals(1.233792E8, (double) conversion.convert(DateUtils.asDateOnly(123456789101L)), 0);
}
{
Conversion conversion = conversionFor(DATETIME, to);
Expand Down Expand Up @@ -282,8 +281,8 @@ public void testConversionToBoolean() {
{
Conversion conversion = conversionFor(DATE, to);
assertNull(conversion.convert(null));
assertEquals(true, conversion.convert(asDate(123456789101L)));
assertEquals(false, conversion.convert(asDate(0L)));
assertEquals(true, conversion.convert(DateUtils.asDateOnly(123456789101L)));
assertEquals(false, conversion.convert(DateUtils.asDateOnly(0L)));
}
{
Conversion conversion = conversionFor(DATETIME, to);
Expand Down Expand Up @@ -329,7 +328,7 @@ public void testConversionToInt() {
{
Conversion conversion = conversionFor(DATE, to);
assertNull(conversion.convert(null));
assertEquals(123379200, conversion.convert(asDate(123456789101L)));
assertEquals(123379200, conversion.convert(DateUtils.asDateOnly(123456789101L)));
}
{
Conversion conversion = conversionFor(DATETIME, to);
Expand Down

0 comments on commit b410903

Please sign in to comment.