SQL: Implement DATETIME_FORMAT function for date/time formatting by matriv · Pull Request #54832 · elastic/elasticsearch

matriv · 2020-04-06T17:29:18Z

Implement DATETIME_FORMAT(<date/datetime/time>, ) function
which allows for formatting a timestamp to the specified format. The patterns
allowed as those of java.time.format.DateTimeFormatter.

Related to #53714

Implement TO_CHAR(<date/datetime/time>, <pattern>) function with aliases: TOCHAR, DATE_FORMAT, FORMAT which allows for formatting a timestamp to the specified format. The patterns allowed as those of `java.time.format.DateTimeFormatter`. Related to elastic#53714

elasticmachine · 2020-04-06T17:29:21Z

Pinging @elastic/es-ql (:Query Languages/SQL)

costin

Left some comments - the only remark I have is about the function name.
Looks good otherwise.

costin · 2020-04-06T19:11:44Z

docs/reference/sql/functions/date-time.asciidoc

+*Description*: Returns the date/datetime/time as a string using the format specified in the 2nd argument. The formatting
+pattern used is the one from
+https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/time/format/DateTimeFormatter.html[`java.time.format.DateTimeFormatter`].
+If any of the two arguments is `null` or the pattern is an empty string a `null` is returned.


drop a : is string null is returned.
If the pattern is empty, null be returned - is that the behavior of the other DBs?

MySQL and PostgreSQL return null for empty pattern, SQL Server returns string in the default format.

Let's go with the PostgreSQL.

costin · 2020-04-06T19:12:22Z

x-pack/plugin/sql/qa/src/main/resources/command.csv-spec

 DOW              |SCALAR         
-DOY              |SCALAR         
+DOY              |SCALAR
+FORMAT           |SCALAR


FORMAT seems a bit too generic - is there any DB that uses it?

costin · 2020-04-06T19:13:27Z

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

+import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isString;
+import static org.elasticsearch.xpack.sql.expression.SqlTypeResolutions.isDateOrTime;
+
+public class ToChar extends BinaryDateTimeFunction {


Why is the function called ToChar and not DateFormat?

No real reason, I just started with the to_char from PostgreSQL.

Will rename it according to the name of the function we decide to use finally.

costin · 2020-04-06T19:21:54Z

docs/reference/sql/functions/date-time.asciidoc

 --------------------------------------------------

+[[sql-functions-datetime-dateformat]]
+==== `DATE_FORMAT/FORMAT/TO_CHAR/TOCHAR`


Do all the function names exist in other DBs? I'd like one which is unique to us due to the Java time format - maybe datetime_format (and datetime_parse) - or vice-versa - format_datetime and parse_datetime.

My point is, I think going forward it would be good to have our own function which uses Java time and then support the rest of the DB functions as translating implementation for the given arguments to our class.
For example DATE_FORMAT which is MySQL specific would exist as such but the MySQL arguments would then be converted into the java.time pattern.
Otherwise looking like MySQL but not supporting its pattern is confusing.

Since the pattern support is tedious, I'm fine with doing that in a separate support for the future. However I'd like to clarify the approach in the docs.

MySQL DATE_FORMAT: https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_date-format
SQL Server FORMAT: https://docs.microsoft.com/en-us/sql/t-sql/functions/format-transact-sql?view=sql-server-ver15#ExampleD
Postgres TO_CHAR: https://www.postgresql.org/docs/9.1/functions-formatting.html
Oracle TOCHAR: https://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#i34510

Note that outside MySQL, the rest of the functions accept multiple formats and data types so to avoid confusion, I would not create aliases for them yet.
But rather treat them separately since if we are to introduce such functions, we would essentially have to obey their semantics rather then ours.

All of DATE_FORMAT, FORMAT and TO_CHAR and TOCHAR are used in DBs and I agree with the confusion regarding the patterns so I'd go for datetime_format/parse with our own ES-SQL impl (java.time).

@astefan @bpintea Do you agree with the naming?

Renamed to DATETIME_FORMAT

Changed now, but it seems sane to me, 👍.

docs/reference/sql/functions/date-time.asciidoc

Co-Authored-By: Bogdan Pintea <bpintea@gmail.com>

bpintea

nicely done.

costin

LGTM

astefan

LGTM. Left just couple of insignificant comments.

astefan · 2020-04-08T00:07:39Z

...n/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/SqlFunctionRegistry.java

                def(MonthName.class, MonthName::new, "MONTH_NAME", "MONTHNAME"),
                def(MonthOfYear.class, MonthOfYear::new, "MONTH_OF_YEAR", "MONTH"),
                def(SecondOfMinute.class, SecondOfMinute::new, "SECOND_OF_MINUTE", "SECOND"),
+                def(DateTimeFormat.class, DateTimeFormat::new, "DATETIME_FORMAT"),


Any particular reason for not using the alphabetical order (as it is now in the registry)?

Good catch, will fix, It was because of the original name ToChar.

astefan · 2020-04-08T00:11:03Z

...sticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeDatePartFunction.java

+                        format(
+                            null,
+                            "first argument of [{}] must be one of {} or their aliases; found value [{}]",
+                            sourceText(),
+                            validDateTimeFieldValues(),
+                            Expressions.name(left())
+                        )


astefan · 2020-04-08T00:16:36Z

...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFormat.java

+        if (resolution.unresolved()) {
+            return resolution;
+        }
+        resolution = isString(right(), sourceText(), Expressions.ParamOrdinal.SECOND);


isString is enough here? (vs. isStringAndExact)

Yep, it's enough, tried for example:

SELECT * FROM test WHERE DATETIME_FORMAT(field1, 'uuuu-MM-dd') = '2020-04-08'; SELECT DATETIME_FORMAT(field1, 'uuuu-MM-dd') FROM test;

on an index:

{ "mappings": { "properties": { "field1": { "type": "date" }, "field2": { "type": "text" } } } }

and data:

{ "field1": "05/04/2020 10:20:30", "field2": "uuuu-MM-dd" }

and it works.

) (#54942) Implement DATETIME_FORMAT(<date/datetime/time>, ) function which allows for formatting a timestamp to the specified format. The patterns allowed as those of java.time.format.DateTimeFormatter. Related to #53714 (cherry picked from commit 72be0b5)

Implement to_char according to the PostgreSQL spec: https://www.postgresql.org/docs/9.1/functions-formatting.html by translating to the java.time patterns used in DATETIME_FORMAT. Follows: elastic#54832

matriv added >feature :Analytics/SQL SQL querying v8.0.0 v7.8.0 labels Apr 6, 2020

matriv requested review from astefan, bpintea and costin April 6, 2020 17:29

fix imports and typo

dfa6515

costin reviewed Apr 6, 2020

View reviewed changes

matriv and others added 3 commits April 7, 2020 15:30

Fix docs

0b46e1d

Rename function and keep only one name

d4a3372

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

525fc28

matriv requested a review from costin April 7, 2020 17:59

bpintea reviewed Apr 7, 2020

View reviewed changes

docs/reference/sql/functions/date-time.asciidoc Outdated Show resolved Hide resolved

matriv and others added 2 commits April 7, 2020 20:48

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

824934f

Update docs/reference/sql/functions/date-time.asciidoc

43af070

Co-Authored-By: Bogdan Pintea <bpintea@gmail.com>

bpintea approved these changes Apr 7, 2020

View reviewed changes

costin approved these changes Apr 7, 2020

View reviewed changes

astefan approved these changes Apr 8, 2020

View reviewed changes

matriv changed the title ~~SQL: Implement TO_CHAR function for date/time formatting~~ SQL: Implement DATETIME_FORMAT function for date/time formatting Apr 8, 2020

Rearrange entry in registry

e11a077

matriv merged commit 72be0b5 into elastic:master Apr 8, 2020

matriv deleted the impl-tochar branch April 8, 2020 09:49

This was referenced Apr 8, 2020

SQL: Implement to_char(date/time/datetime, pattern) #54964

Closed

SQL: Implement FORMAT function #54965

Closed

SQL: Implement DATE_FORMAT function #55065

Closed

Anyrob mentioned this pull request Jun 30, 2020

#55065: Add DATEFORMAT function. #58711

Closed

vonsowic mentioned this pull request Oct 15, 2020

SQL: Implement to_char function (#54964) #63766

Closed

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

Conversation

matriv commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 6, 2020

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

matriv commented Apr 6, 2020 •

edited

Loading