Address comments Pt3

matriv · matriv · commit 7409ce46df66 · 2019-01-22T16:58:39.000+02:00
diff --git a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcDateUtils.java b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcDateUtils.java
@@ -12,7 +12,6 @@
 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;
 
@@ -43,25 +42,8 @@ final class JdbcDateUtils {
         .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;
-        try {
-            zdt = ISO_WITH_MILLIS.parse(date, ZonedDateTime::from);
-        } catch (DateTimeParseException e) {
-            // Case of a Group By with casting as DATE
-            zdt = ISO_WITH_MINUTES.parse(date, ZonedDateTime::from);
-        }
-        return zdt.toInstant().toEpochMilli();
+        return ISO_WITH_MILLIS.parse(date, ZonedDateTime::from).toInstant().toEpochMilli();
     }
     
     static Date asDate(String date) {
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/CompositeKeyExtractor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/CompositeKeyExtractor.java
@@ -96,6 +96,8 @@ public Object extract(Bucket bucket) {
                 return object;
             } else if (object instanceof Long) {
                 object = DateUtils.asDateTime(((Long) object).longValue(), zoneId);
+            } else if (object instanceof String) { // CAST(<value> AS DATE) is used
+                object = DateUtils.asDateOnly(object.toString());
             } else {
                 throw new SqlIllegalArgumentException("Invalid date key returned: {}", object);
             }
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java
@@ -22,8 +22,6 @@
 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 {
 
@@ -174,11 +172,11 @@ public static TypeResolution typeMustBeString(Expression e, String operationName
     }
 
     public static TypeResolution typeMustBeDate(Expression e, String operationName, ParamOrdinal paramOrd) {
-        return typeMustBe(e, dt -> dt == DATETIME || dt == DATE, operationName, paramOrd, "datetime, date");
+        return typeMustBe(e, DataType::isDate, operationName, paramOrd, "datetime, date");
     }
 
     public static TypeResolution typeMustBeNumericOrDate(Expression e, String operationName, ParamOrdinal paramOrd) {
-        return typeMustBe(e, dt -> dt.isNumeric() || dt == DATETIME, operationName, paramOrd, "numeric", "datetime", "date");
+        return typeMustBe(e, dt -> dt.isNumeric() || dt.isDate(), operationName, paramOrd, "numeric", "datetime", "date");
     }
 
     public static TypeResolution typeMustBe(Expression e,
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
@@ -357,7 +357,7 @@ private static Object asDateTime(Object dateTime, boolean lenient) {
             return ((JodaCompatibleZonedDateTime) dateTime).getZonedDateTime();
         }
         if (dateTime instanceof ZonedDateTime) {
-            return (ZonedDateTime) dateTime;
+            return dateTime;
         }
         if (false == lenient) {
             if (dateTime instanceof Number) {
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/ScriptWeaver.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/ScriptWeaver.java
@@ -79,7 +79,7 @@ default ScriptTemplate scriptWithScalar(ScalarFunctionAttribute scalar) {
 
     default ScriptTemplate scriptWithAggregate(AggregateFunctionAttribute aggregate) {
         String template = "{}";
-        if (aggregate.dataType() == DataType.DATETIME) {
+        if (aggregate.dataType().isDate()) {
             template = "{sql}.asDateTime({})";
         }
         return new ScriptTemplate(processScript(template),
@@ -89,7 +89,7 @@ default ScriptTemplate scriptWithAggregate(AggregateFunctionAttribute aggregate)
 
     default ScriptTemplate scriptWithGrouping(GroupingFunctionAttribute grouping) {
         String template = "{}";
-        if (grouping.dataType() == DataType.DATETIME) {
+        if (grouping.dataType().isDate()) {
             template = "{sql}.asDateTime({})";
         }
         return new ScriptTemplate(processScript(template),
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java
@@ -61,7 +61,6 @@
 import org.elasticsearch.xpack.sql.rule.Rule;
 import org.elasticsearch.xpack.sql.rule.RuleExecutor;
 import org.elasticsearch.xpack.sql.session.EmptyExecutable;
-import org.elasticsearch.xpack.sql.type.DataType;
 import org.elasticsearch.xpack.sql.util.Check;
 import org.elasticsearch.xpack.sql.util.DateUtils;
 
@@ -284,7 +283,7 @@ protected PhysicalPlan rule(AggregateExec a) {
                                 if (matchingGroup != null) {
                                     if (exp instanceof Attribute || exp instanceof ScalarFunction || exp instanceof GroupingFunction) {
                                         Processor action = null;
-                                        ZoneId zi = DataType.DATETIME == exp.dataType() ? DateUtils.UTC : null;
+                                        ZoneId zi = exp.dataType().isDate() ? DateUtils.UTC : null;
                                         /*
                                          * special handling of dates since aggs return the typed Date object which needs
                                          * extraction instead of handling this in the scroller, the folder handles this
@@ -335,7 +334,7 @@ protected PhysicalPlan rule(AggregateExec a) {
                                 // check if the field is a date - if so mark it as such to interpret the long as a date
                                 // UTC is used since that's what the server uses and there's no conversion applied
                                 // (like for date histograms)
-                                ZoneId zi = DataType.DATETIME == child.dataType() ? DateUtils.UTC : null;
+                                ZoneId zi = child.dataType().isDate() ? DateUtils.UTC : null;
                                 queryC = queryC.addColumn(new GroupByRef(matchingGroup.id(), null, zi));
                             }
                             // handle histogram
@@ -359,7 +358,7 @@ else if (child instanceof GroupingFunction) {
                             matchingGroup = groupingContext.groupFor(ne);
                             Check.notNull(matchingGroup, "Cannot find group [{}]", Expressions.name(ne));
 
-                            ZoneId zi = DataType.DATETIME == ne.dataType() ? DateUtils.UTC : null;
+                            ZoneId zi = ne.dataType().isDate() ? DateUtils.UTC : null;
                             queryC = queryC.addColumn(new GroupByRef(matchingGroup.id(), null, zi));
                         }
                     }
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/GroupByKey.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/GroupByKey.java
@@ -39,7 +39,7 @@ public final CompositeValuesSourceBuilder<?> asValueSource() {
                 builder.valueType(ValueType.DOUBLE);
             } else if (script.outputType().isString()) {
                 builder.valueType(ValueType.STRING);
-            } else if (script.outputType() == DataType.DATETIME) {
+            } else if (script.outputType().isDate()) {
                 builder.valueType(ValueType.DATE);
             } else if (script.outputType() == DataType.BOOLEAN) {
                 builder.valueType(ValueType.BOOLEAN);
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java
@@ -215,6 +215,10 @@ public boolean isString() {
     public boolean isPrimitive() {
         return this != OBJECT && this != NESTED;
     }
+
+    public boolean isDate() {
+        return this == DATE || this == DATETIME;
+    }
     
     public static DataType fromOdbcType(String odbcType) {
         return odbcToEs.get(odbcType);

Original file line number	Diff line number	Diff line change
`@@ -96,6 +96,8 @@ public Object extract(Bucket bucket) {`
`96`	`96`	`return object;`
`97`	`97`	`} else if (object instanceof Long) {`
`98`	`98`	`object = DateUtils.asDateTime(((Long) object).longValue(), zoneId);`
	`99`	`+ } else if (object instanceof String) { // CAST(<value> AS DATE) is used`
	`100`	`+ object = DateUtils.asDateOnly(object.toString());`
`99`	`101`	`} else {`
`100`	`102`	`throw new SqlIllegalArgumentException("Invalid date key returned: {}", object);`
`101`	`103`	`}`
Original file line number	Diff line number	Diff line change
`@@ -22,8 +22,6 @@`
`22`	`22`	`import static java.util.Collections.emptyList;`
`23`	`23`	`import static java.util.Collections.emptyMap;`
`24`	`24`	`import static org.elasticsearch.xpack.sql.type.DataType.BOOLEAN;`
`25`		`-import static org.elasticsearch.xpack.sql.type.DataType.DATE;`
`26`		`-import static org.elasticsearch.xpack.sql.type.DataType.DATETIME;`
`27`	`25`
`28`	`26`	`public final class Expressions {`
`29`	`27`
`@@ -174,11 +172,11 @@ public static TypeResolution typeMustBeString(Expression e, String operationName`
`174`	`172`	`}`
`175`	`173`
`176`	`174`	`public static TypeResolution typeMustBeDate(Expression e, String operationName, ParamOrdinal paramOrd) {`
`177`		`- return typeMustBe(e, dt -> dt == DATETIME \|\| dt == DATE, operationName, paramOrd, "datetime, date");`
	`175`	`+ return typeMustBe(e, DataType::isDate, operationName, paramOrd, "datetime, date");`
`178`	`176`	`}`
`179`	`177`
`180`	`178`	`public static TypeResolution typeMustBeNumericOrDate(Expression e, String operationName, ParamOrdinal paramOrd) {`
`181`		`- return typeMustBe(e, dt -> dt.isNumeric() \|\| dt == DATETIME, operationName, paramOrd, "numeric", "datetime", "date");`
	`179`	`+ return typeMustBe(e, dt -> dt.isNumeric() \|\| dt.isDate(), operationName, paramOrd, "numeric", "datetime", "date");`
`182`	`180`	`}`
`183`	`181`
`184`	`182`	`public static TypeResolution typeMustBe(Expression e,`
Original file line number	Diff line number	Diff line change
`@@ -357,7 +357,7 @@ private static Object asDateTime(Object dateTime, boolean lenient) {`
`357`	`357`	`return ((JodaCompatibleZonedDateTime) dateTime).getZonedDateTime();`
`358`	`358`	`}`
`359`	`359`	`if (dateTime instanceof ZonedDateTime) {`
`360`		`- return (ZonedDateTime) dateTime;`
	`360`	`+ return dateTime;`
`361`	`361`	`}`
`362`	`362`	`if (false == lenient) {`
`363`	`363`	`if (dateTime instanceof Number) {`