SQL: Implement DATE_PART function #47206

matriv · 2019-09-27T10:09:53Z

DATE_PART(, <date/datetime>) is a function that allows
the user to extract the specified unit from a date/datetime field
similar to the EXTRACT ( FROM <date/datetime>) but
with different names and aliases for the units and it also provides more
options like DATE_PART('tzoffset', datetimeField).

Implemented following the SQL server's spec: https://docs.microsoft.com/en-us/sql/t-sql/functions/datepart-transact-sql?view=sql-server-2017
with the difference that the argument is either a
literal single quoted string or gets a value from a table field, whereas
in SQL server keywords are used (unquoted identifiers) and it's not
possible to use a value coming for a table column.

Closes: #46372

DATE_PART(<datetime unit>, <date/datetime>) is a function that allows the user to extract the specified unit from a date/datetime field similar to the EXTRACT (<datetime unit> FROM <date/datetime>) but with different names and aliases for the units and it also provides more options like `DATE_PART('tzoffset', datetimeField)`. Implemented following the SQL server's spec: https://docs.microsoft.com/en-us/sql/t-sql/functions/datepart-transact-sql?view=sql-server-2017 with the difference that the <datetime unit> argument is either a literal single quoted string or gets a value from a table field, whereas in SQL server keywords are used (unquoted identifiers) and it's not possible to use a value coming for a table column. Closes: elastic#46372

elasticmachine · 2019-09-27T10:09:55Z

Pinging @elastic/es-search

matriv · 2019-09-27T10:10:55Z

This implementation is missing the 3rd argument where the Sunday or Monday can be specified.
It will be added with a followup PR.

matriv · 2019-09-27T10:52:32Z

@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/docs-check

costin

LGTM

costin · 2019-09-27T11:42:14Z

...java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimePipe.java

-        return zoneId.equals(that.zoneId);
+        BinaryDateTimePipe that = (BinaryDateTimePipe) o;
+        return Objects.equals(zoneId, that.zoneId) &&
+            operation == that.operation;
    }

    @Override
    public int hashCode() {


Please move hashCode before equals.

costin · 2019-09-27T11:44:29Z

.../src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePart.java

+        HOUR(DateTimeExtractor.HOUR_OF_DAY::extract, "hours", "hh"),
+        MINUTE(DateTimeExtractor.MINUTE_OF_HOUR::extract, "minutes", "mi", "n"),
+        SECOND(DateTimeExtractor.SECOND_OF_MINUTE::extract, "seconds", "ss", "s"),
+        MILLISECOND(dt -> {


dt -> dt.get(...)

costin · 2019-09-27T11:50:32Z

.../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePartProcessor.java

+            }
+        }
+
+        if (!(source2 instanceof ZonedDateTime)) {


costin · 2019-09-27T11:54:02Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/IdentifierBuilder.java

@@ -45,4 +45,8 @@ public String visitQualifiedName(QualifiedNameContext ctx) {
    private static String unquoteIdentifier(String identifier) {
        return identifier.replace("\"\"", "\"");
    }
+
+    static String quoteIdentifier(String identifier) {


Where is this used?

Nowhere, must be removed, thx!

costin · 2019-09-27T11:59:35Z

...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java

@@ -110,9 +103,11 @@
        NANOSECOND(dt -> dt, "nanoseconds", "ns");

        private static final Map<String, Part> NAME_TO_PART;
+        private static final List<String> VALID_VALUES;


Potentially the findSimilar could be extracted into an interface with default methods (no need for static maps considering there's only one enum instance per ClassLoader).

astefan

Great work. Left some comments.

astefan · 2019-10-01T09:04:51Z

.../org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeFunction.java

+        }
+
+        if (left().foldable()) {
+            String truncateToValue = (String) left().fold();


Why is this variable called like this? truncateToValue

Because of some refactoring, and extraction of an abstract class, fixing.

astefan · 2019-10-01T09:07:08Z

.../org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeFunction.java

+    public Object fold() {
+        if (operation == TRUNC) {
+            return DateTruncProcessor.process(left().fold(), right().fold(), zoneId);
+        } else {


I would add a comment here or for the entire class pointing out that, for the moment, this class has only two subclasses - datepart and datetrunc - thus, the else branch points directly to DatePartProcessor.

astefan · 2019-10-01T09:16:18Z

.../org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeFunction.java

+    @Override
+    protected ScriptTemplate asScriptFrom(ScriptTemplate leftScript, ScriptTemplate rightScript) {
+        return new ScriptTemplate(
+            formatTemplate("{sql}." + (operation == TRUNC ? "dateTrunc" : "datePart") +


Maybe refactor operation == TRUNC or, preferable, the entire TRUNC/PART logic in the subclasses themselves so that the abstract class only uses abstract methods that have definitions in the subclasses.

For example, in the asScriptFrom method don't use the operation == TRUNC ? "dateTrunc" : "datePart" logic, but delegate the decision to use either dateTrunc or datePart string to an abstract method belonging to both DatePart and DateTrunc classes where they return datePart and dateTrunc strings.

And I would say the same should be valid for fold() method where the same comparison is being made.

astefan · 2019-10-01T09:24:33Z

...org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeProcessor.java

+    protected abstract Object doProcess(Object left, Object right);
+
+    public static BinaryDateTimeProcessor asProcessor(BinaryDateOperation operation, Processor left, Processor right, ZoneId zoneId) {
+        if (operation == TRUNC) {


Same comment here. Why not delegating the choice of which processor to return to the subclasses themselves, as they are the decision makers. What if this abstract BinaryDateTimeProcessor will have another subclass in the future? It's weird to come back to the base class and change its implementation based on new subclasses that were added. Rather the subclasses should provide its specific implementation of a certain aspect of code and the base class should remain unchanged.

astefan · 2019-10-01T09:28:31Z

.../src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePart.java

+            VALID_VALUES = DateTimeField.initializeValidValues(values());
+        }
+
+        private Set<String> aliases;


I would change the definition order to be extractFunction, aliases to match the constructor parameters order and initialization.

astefan · 2019-10-01T09:30:30Z

.../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePartProcessor.java

+        if (source1 == null || source2 == null) {
+            return null;
+        }
+        if (!(source1 instanceof String)) {


I would prefer source1 instanceof String == false.

astefan · 2019-10-01T09:32:08Z

.../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePartProcessor.java

+                throw new SqlIllegalArgumentException("A value of {} or their aliases is required; received [{}]",
+                    Part.values(), source1);
+            } else {
+                throw new SqlIllegalArgumentException("Received value [{}] is not valid date part for extraction; " + "" +


Why an emtpy string concatenation here? + "" +

Left over from splitting to new line.

astefan · 2019-10-01T09:33:42Z

.../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePartProcessor.java

+        }
+
+        if (source2 instanceof ZonedDateTime == false) {
+            throw new SqlIllegalArgumentException("A datetime/date is required; received [{}]", source2);


The documentation mentions date/datetime while the error message is about datetime/date. Small, picky comment, but I would try to be consistent.

No worries for "picky" comment, we should be consistent, thx for the catch.

astefan · 2019-10-01T09:35:40Z

...main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeField.java

+public interface DateTimeField {
+
+    static <D extends DateTimeField> Map<String, D> initializeResolutionMap(D[] values) {
+        Map<String, D> nameToPart;


Why not a single line Map<String, D> nameToPart = new HashMap<>(); instead of two lines?

astefan · 2019-10-01T09:51:25Z

...org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimePipeTests.java

                newLoc,
                b2.expression(),
                b2.left(),
                b2.right(),
-                b2.zoneId());
+                b2.zoneId(),
+                b1.operation());


Shouldn't here be b2.operation()?

Nice catch! thx!

matriv · 2019-10-01T11:36:04Z

@astefan Thank you for the thorough review. Please check again.

astefan

LGTM

DATE_PART(<datetime unit>, <date/datetime>) is a function that allows the user to extract the specified unit from a date/datetime field similar to the EXTRACT (<datetime unit> FROM <date/datetime>) but with different names and aliases for the units and it also provides more options like `DATE_PART('tzoffset', datetimeField)`. Implemented following the SQL server's spec: https://docs.microsoft.com/en-us/sql/t-sql/functions/datepart-transact-sql?view=sql-server-2017 with the difference that the <datetime unit> argument is either a literal single quoted string or gets a value from a table field, whereas in SQL server keywords are used (unquoted identifiers) and it's not possible to use a value coming for a table column. Closes: #46372 (cherry picked from commit ead743d)

Add a 3rd argument to the DATE_PART function which is optional and can take one of the `['Monday', 'Sunday']` values. If missing then `Sunday` is used by default. This value is used for the `week` and `weekday` extraction to allow for ISO-style calculation of the two fields if the value `Monday` is used. Follows: elastic#47206

matriv added :Analytics/SQL SQL querying v8.0.0 v7.5.0 labels Sep 27, 2019

matriv requested review from costin, astefan and bpintea September 27, 2019 10:09

matriv added the >feature label Sep 27, 2019

matriv added 2 commits September 27, 2019 13:21

Add examples for tzoffset

b9dd2cb

Add comment for ignored tzoffset tests

65d6949

costin approved these changes Sep 27, 2019

View reviewed changes

matriv added 3 commits September 27, 2019 16:03

Address comments

2ce6182

Merge remote-tracking branch 'upstream/master' into impl-46372

2314f8c

Refactor to share code between enums

abdbfa6

astefan reviewed Oct 1, 2019

View reviewed changes

matriv and others added 2 commits October 1, 2019 14:25

Merge remote-tracking branch 'upstream/master' into impl-46372

c8d39c9

Address comments

de67bf7

matriv requested a review from astefan October 1, 2019 11:36

astefan approved these changes Oct 1, 2019

View reviewed changes

matriv merged commit ead743d into elastic:master Oct 1, 2019

matriv deleted the impl-46372 branch October 1, 2019 12:59

matriv mentioned this pull request Oct 4, 2019

SQL: Add 3rd optional arg to DATE_PART #47614

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Implement DATE_PART function #47206

SQL: Implement DATE_PART function #47206

matriv commented Sep 27, 2019

elasticmachine commented Sep 27, 2019

matriv commented Sep 27, 2019

matriv commented Sep 27, 2019

costin left a comment

costin Sep 27, 2019

costin Sep 27, 2019

costin Sep 27, 2019

costin Sep 27, 2019

matriv Sep 27, 2019

costin Sep 27, 2019

astefan left a comment

astefan Oct 1, 2019

matriv Oct 1, 2019

astefan Oct 1, 2019

astefan Oct 1, 2019

astefan Oct 1, 2019

astefan Oct 1, 2019

astefan Oct 1, 2019

astefan Oct 1, 2019

matriv Oct 1, 2019

astefan Oct 1, 2019

matriv Oct 1, 2019

astefan Oct 1, 2019

astefan Oct 1, 2019

matriv Oct 1, 2019

matriv commented Oct 1, 2019

astefan left a comment

SQL: Implement DATE_PART function #47206

SQL: Implement DATE_PART function #47206

Conversation

matriv commented Sep 27, 2019

elasticmachine commented Sep 27, 2019

matriv commented Sep 27, 2019

matriv commented Sep 27, 2019

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Oct 1, 2019

astefan left a comment

Choose a reason for hiding this comment