SQL: Fix FORMAT function to better comply with Microsoft SQL Server specification #86225

luigidellaquila · 2022-04-27T14:18:14Z

Based on the documentation, FORMAT() function should comply with Microsoft SQL Server Format specification.

Current implementation uses Java DateTimeFormat as it is, only with replacement of basic pattern characters that are slightly different between the two specs, but it's not fully compliant.

The goal of this PR is to better translate Microsoft datetime patterns to the equivalent in Java, and addresses the cases that are not currently covered, ie.

allowed characters in Microsoft spec that are not allowed in Java are now quoted and returned correctly
quoting syntax (that is different in the two specs) is properly translated
some further fixes to patterns translation (eg. Y and y in Java are equivalent, while in MS spec Y is just a normal character, not interpreted as a pattern)

Fixes #66560

elasticmachine · 2022-04-27T14:18:18Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2022-04-27T14:19:01Z

Hi @luigidellaquila, I've created a changelog YAML for you.

luigidellaquila · 2022-04-27T14:25:17Z

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

@@ -136,39 +136,39 @@ SELECT ISO_DAY_OF_WEEK(birth_date) AS d, SUM(salary) s FROM test_emp GROUP BY d

      d:i      |      s:l
 ---------------+---------------
-7              |386466       


Sorry for this, it's annoying for me as well...
IntelliJ keeps trimming blank spaces in csv-spec files on save, and there is apparently no way to avoid it, even disabling it from the IDE settings (not sure if it's a mix of IntelliJ + checkstyle + OSX).

@luigidellaquila this is probably due to .editorconfig. You could add some further config to it, which worked when I tested it:

diff --git a/.editorconfig b/.editorconfig index 92111b6c074..00dc2f6fa83 100644 --- a/.editorconfig +++ b/.editorconfig @@ -225,3 +225,6 @@ indent_size = 2 [*.{xsd,xml}] indent_size = 4 + +[*.{csv,sql}-spec] +trim_trailing_whitespace = false

Thank you very much @pugnascotia, I'll try it

luigidellaquila · 2022-04-27T14:29:53Z

@elasticmachine update branch

astefan

Left some comments.
Also, I suggest changing the title of the PR to Fix FORMAT function to better comply with Microsoft SQL Server specification. Not all the characteristics of MS format are supported.

astefan · 2022-04-28T09:39:44Z

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

+                    // manage patterns that are different from MS to Java and have to be translated
+                    boolean replaced = false;
+                    for (String[] item : MS_TO_JAVA_PATTERNS) {
+                        if (i + item[0].length() <= pattern.length() && item[0].equals(pattern.substring(i, i + item[0].length()))) {


Extract item[0].length() outside if and call it only once.

astefan · 2022-04-28T09:41:12Z

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

@@ -67,6 +86,105 @@ protected Function<TemporalAccessor, String> formatterFor(String pattern) {
            }
        };

+        private static String msToJavaPattern(String pattern) {


I think this method should be unit tested. There are many edge cases with translating one format to another and we need to be extra careful with them and make sure everything is covered. Make this method package protected and add tests for it.

astefan · 2022-04-28T09:44:41Z

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

+            StringBuilder partialQuotedString = new StringBuilder();
+
+            boolean originQuoting = false;
+            boolean targetQuoting = false;


Better named as shouldQuoteTarget?

I see, the naming is bad and misleading.
What I mean here is:

originQuoting: the character in the original (MS) pattern was intended to be quoted (eg. previous char was a backslash or is inside a quoting block).

targetQuoting: last character in the generated (java) pattern has to be quoted, so I'm building a portion of pattern that will then be quoted. Unfortunately I cannot quote characters one by one, I have to accumulate portions of string that have to be quoted and then quote them properly

I'll try to find better names

astefan · 2022-04-28T09:47:14Z

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

+            StringBuilder result = new StringBuilder(pattern.length());
+            StringBuilder partialQuotedString = new StringBuilder();
+
+            boolean originQuoting = false;


Better named as isStartQuote?

astefan · 2022-04-28T09:49:39Z

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

+                result.append("'");
+                result.append(partialQuotedString.toString().replaceAll("'", "''"));
+                result.append("'");


Extract this as a small method and use it here and above as well.

astefan · 2022-04-28T10:25:24Z

...lasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFormatProcessorTests.java

        );
        assertEquals(
            "2019-09-04 04:10:37.12345678 AM",
-            new Format(Source.EMPTY, dateTime, l("YYYY-MM-dd HH:mm:ss.ffffffff t"), zoneId).makePipe().asProcessor().process(null)


I would also add a test with YYYY to show that the MS pattern is case-sensitive.

Added above

- code refactoring - further unit tests - fix .editorconfig to avoid trim of trailing whitespaces in csv-spec files

cla-checker-service · 2022-04-28T11:15:53Z

💚 CLA has been signed

Luegg

Are there more breaking changes apart from Y vs. y?

For Y vs y I would suggest to keep supporting upper case Y and potentially emit a deprecation warning instead of breaking current behavior. Since even the docs uses Y in the example it seems quite likely that users also do so.

luigidellaquila · 2022-04-28T12:06:55Z

@elasticmachine run CLA

luigidellaquila · 2022-04-28T12:16:24Z

@Luegg yes, there are many breaking changes here, like A, N, G, VV...
Actually, with the original implementation, only nine MS patterns were translated to Java (see diff), all the rest of the characters were just copied and interpreted with Java semantics. As you can see, there are maybe ten or more characters that Java DateTimeFormatters interprets as valid date patterns and that MS just ignores.

IMHO it could still make sense to treat Y as an exception, since it's pretty common, but probably we should not maintain the compatibility with all the other cases.

astefan · 2022-04-28T12:16:49Z

For Y vs y I would suggest to keep supporting upper case Y and potentially emit a deprecation warning instead of breaking current behavior. Since even the docs uses Y in the example it seems quite likely that users also do so.

I have mixed feelings about this. This is a bug and should be treated like it in my opinion: changing the output means fixing a faulty behavior in the first place. On the other hand, we could do as you suggest and be lenient towards such changes, but this could introduce a precedent: every bug fix should also consider any (if it wouldn't be a bug fix) introduced breaking changes and add deprecation warnings and so on. @bpintea @costin wdyt?

costin

LGTM.
Left some comments around the coding style to replace the majority of if .. continue with else if.
Regarding the changes, I don't think we need to be lenient - this function is specific to SQL Server and should act accordingly. The fact that Y has been supported instead of y is a bug - if folks want to use the Java formatter rules, they can do so using the regular DATE_FORMATTER or simply change their queries to use y instead. It is annoying however it has been a bug that affected also our documentation.
In short, I would just apply the changes without any special treatment of the previous behavior.

costin · 2022-04-28T15:32:15Z

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

+                        originalCharacterQuoted = false;
+                        lastTargetCharacterQuoted = true;
+                        partialQuotedString.append(c);
+                        continue;


Please use else if instead of continue through-out this method.

Luegg · 2022-04-28T16:03:26Z

Interesting, I wasn't aware that we support 3 different formats for date formatting and assumed that's the main one.

In any case, there is one more occurrence of the upper case Y in the docs:

astefan

LGTM

bpintea · 2022-05-16T18:22:03Z

I'm wondering just how close to the MS specification we will want to get?
I'm thinking of the treatment of %, which has this single custom format specifier semantic, or single " characters, which are just piped through by MS's FORMAT, but swallowed by current implementation, or double tick '', that will be piped through by MS's FORMAT, but treated as a single ' by DateTimeFormatter.

luigidellaquila · 2022-05-17T14:28:40Z

I'm wondering just how close to the MS specification we will want to get?

@bpintea MS and Java formats are not different only in the syntax, but also on some concepts. One of them is custom format specifiers, that don't seem to have an equivalent in Java.
The goal here is to have has much compliance with MS specs as possible, especially on the allowed characters (see #66560), while preserving current implementation based on Java DateTimeFormatter.
Some concepts, like those you mentioned, would require a more complex effort (probably a complete rewrite of the formatter), this is why I decided to leave them out of the scope of the PR

bpintea · 2022-05-17T17:41:31Z

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

+                                // and then translate the pattern
+                                result.append(item[1]);
+                                characterProcessed = true;
+                                i += (fragmentLength - 1); // fast-forward, because the replaced pattern could be longer than one character


Nit: the parentheses shouldn't be needed. Just in case any other changes will be needed.
Also, I'm generally in favour of using a while loop and explicitly incrementing by needed amount (vs. decrementing due to for).

bpintea

LGTM

Fix SQL FORMAT function to comply with Microsoft SQL Server spec

fe37278

luigidellaquila added >bug :Analytics/SQL SQL querying labels Apr 27, 2022

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Apr 27, 2022

luigidellaquila requested a review from Luegg April 27, 2022 14:18

elasticsearchmachine added the v8.3.0 label Apr 27, 2022

luigidellaquila requested review from astefan, costin and bpintea April 27, 2022 14:18

Update docs/changelog/86225.yaml

4a86322

luigidellaquila commented Apr 27, 2022

View reviewed changes

Merge branch 'master' into fix/format_date_fix

5ae188a

astefan reviewed Apr 28, 2022

View reviewed changes

luigidellaquila changed the title ~~SQL: Fix FORMAT function to comply with Microsoft SQL Server specification~~ SQL: Fix FORMAT function to better comply with Microsoft SQL Server specification Apr 28, 2022

Fix based on review comments

5decccd

- code refactoring - further unit tests - fix .editorconfig to avoid trim of trailing whitespaces in csv-spec files

Luegg reviewed Apr 28, 2022

View reviewed changes

costin approved these changes Apr 28, 2022

View reviewed changes

Refactor DateTimeFormatProcessor and fix docs

450db30

luigidellaquila requested review from astefan and Luegg May 10, 2022 12:17

astefan approved these changes May 11, 2022

View reviewed changes

bpintea reviewed May 17, 2022

View reviewed changes

bpintea approved these changes May 17, 2022

View reviewed changes

Luegg approved these changes May 18, 2022

View reviewed changes

luigidellaquila merged commit f69c739 into elastic:master May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Fix FORMAT function to better comply with Microsoft SQL Server specification #86225

SQL: Fix FORMAT function to better comply with Microsoft SQL Server specification #86225

luigidellaquila commented Apr 27, 2022

elasticmachine commented Apr 27, 2022

elasticsearchmachine commented Apr 27, 2022

luigidellaquila Apr 27, 2022

pugnascotia Apr 28, 2022

luigidellaquila Apr 28, 2022

luigidellaquila commented Apr 27, 2022

astefan left a comment

astefan Apr 28, 2022

astefan Apr 28, 2022

astefan Apr 28, 2022

luigidellaquila Apr 28, 2022

astefan Apr 28, 2022

astefan Apr 28, 2022

astefan Apr 28, 2022

luigidellaquila Apr 28, 2022

cla-checker-service bot commented Apr 28, 2022 •

edited

Loading

Luegg left a comment

luigidellaquila commented Apr 28, 2022

luigidellaquila commented Apr 28, 2022 •

edited

Loading

astefan commented Apr 28, 2022

costin left a comment

costin Apr 28, 2022

Luegg commented Apr 28, 2022

astefan left a comment

bpintea commented May 16, 2022

luigidellaquila commented May 17, 2022

bpintea May 17, 2022

bpintea left a comment

SQL: Fix FORMAT function to better comply with Microsoft SQL Server specification #86225

SQL: Fix FORMAT function to better comply with Microsoft SQL Server specification #86225

Conversation

luigidellaquila commented Apr 27, 2022

elasticmachine commented Apr 27, 2022

elasticsearchmachine commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luigidellaquila commented Apr 27, 2022

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

cla-checker-service bot commented Apr 28, 2022 • edited Loading

Luegg left a comment

Choose a reason for hiding this comment

luigidellaquila commented Apr 28, 2022

luigidellaquila commented Apr 28, 2022 • edited Loading

astefan commented Apr 28, 2022

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luegg commented Apr 28, 2022

astefan left a comment

Choose a reason for hiding this comment

bpintea commented May 16, 2022

luigidellaquila commented May 17, 2022

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

cla-checker-service bot commented Apr 28, 2022 •

edited

Loading

luigidellaquila commented Apr 28, 2022 •

edited

Loading