[CALCITE-6551] Add DATE_FORMAT function (enabled in MySQL library) #3936

suibianwanwank · 2024-08-28T16:09:21Z

Document: https://dev.mysql.com/doc/refman/8.4/en/date-and-time-functions.html#function_date-format

suibianwanwank · 2024-08-28T16:11:15Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+      f.checkString("date_format(timestamp '1997-10-04 22:23:00.000000', '%c %D %e %f %h')",
+          "10 4th 4 000000 10",
+          "VARCHAR NOT NULL");
+      f.checkString("date_format(timestamp '1997-01-04 22:23:00', '%w %U %u %V %v %X %x')",


Here are 4 cases testing mysql's weekForYear in two modes.
https://stackoverflow.com/questions/30364141/mysql-convert-yearweek-to-date

suibianwanwank · 2024-08-28T16:16:20Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+//      f.checkString("date_format('1999-13-01', '%X %V')",
+//          null,


MySQL supports this writing and will return null if the timestamp type is not declared and the parameter does not satisfy the timestamp format, which seems to be difficult to do in calcite

what you need is to use an implicit cast to timestamp if the input is a CHAR data type.
you should be able to support that.

Yes, implicit cast to timestamp this can be done in calcite, but if this cast fails, calcite will throw an exception, but for mysql, it will return null.
This is something that I don't think is easy to implement. unless we add a Config that adds a Config in the checksum phase and returns NULL if the implicit cast fails. WDYT

I see, the date is incorrect.
In this case indeed you can probably not match exactly the behavior of mysql.
But I think you should still make it a negative test and leave the comment in.

suibianwanwank · 2024-08-28T16:17:38Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

@@ -4919,7 +4920,8 @@ void testBitGetFunc(SqlOperatorFixture f, String functionName) {
  }

  @Test void testToCharPg() {
-    final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.POSTGRESQL);


I noticed that the default tester doesn't check the results, so I added this.

We check the results by CalciteSqlOperatorTest. In SqlOperatorTest we check the return type.

suibianwanwank · 2024-08-28T17:58:10Z

The local time zone causes incorrect results and I need to check how to fix it.

mihaibudiu · 2024-08-28T17:59:59Z

If you want deterministic tests I think you should not use the local timezone.

asolimando · 2024-08-28T19:41:04Z

If you want deterministic tests I think you should not use the local timezone.

@mihaibudiu is right, tests should be timezone agnostic, see Cassandra tests for an example.

suibianwanwank · 2024-08-29T01:13:25Z

@mihaibudiu @asolimando I found this problem with all of this test classes, if the time zone is changed arbitrarily. For Calender's test, we need to set the time zone at this time like SqlOperatorTest. I'm not sure if this should be fixed in this PR .

mihaibudiu · 2024-08-29T01:15:42Z

You can see a solution in e.g., #3805, where the timezone is set before the tests and reset afterwards. It's sufficient to make your own tests pass.

suibianwanwank · 2024-08-29T06:54:20Z

You can see a solution in e.g., #3805, where the timezone is set before the tests and reset afterwards. It's sufficient to make your own tests pass.

I've fixed my test by keeping the data consistent with other tests. But I think in some time zones, some tests may not pass if we don't specify the time zone.

mihaibudiu · 2024-08-29T17:52:30Z

core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java

+    @Override public void format(StringBuilder sb, Date date) {
+      final Calendar calendar = Work.get().calendar;
+      calendar.setTime(date);
+      // It exceeds the precision of Calendar, we fill it with 0.


Milliseconds are usually between 0 and 999.

This is an incorrect function description, it means microsecond here, I'll fix it

mihaibudiu · 2024-08-29T17:54:01Z

core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java

@@ -221,6 +245,33 @@ public enum FormatElementEnum implements FormatElement {
      sb.append(String.format(Locale.ROOT, "%02d", calendar.get(Calendar.WEEK_OF_YEAR)));
    }
  },
+  V("", "The number of the year (Monday as the first day of the week),"


the comment seems wrong, it is about the week number

+1， i will fix it

mihaibudiu · 2024-08-29T17:54:36Z

core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java

@@ -399,6 +480,27 @@ public enum FormatElementEnum implements FormatElement {
      sb.append(work.yyyyFormat.format(date));
    }
  },
+  WFY("", "The year for the week where Sunday is the first day of the week") {


I think this should say "the week of the year"

The result returned by this symbol is the number of years, "the week of the year" might not be appropriate

mihaibudiu · 2024-08-29T17:54:50Z

core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java

+      sb.append(String.format(Locale.ROOT, "%04d", calendar.getWeekYear()));
+    }
+  },
+  WFY0("", "The year for the week where Monday is the first day of the week") {


same problem

mihaibudiu · 2024-08-29T17:56:31Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+    final Locale originalLocale = Locale.getDefault();
+
+    try {
+      Locale.setDefault(Locale.US);


You should not need to set the locale in the test if these functions are supposed to be deterministic.
Perhaps the function implementation should set the locale.

For datetime functions, the implementation should be based on the user's time zone, just as other arbitrary databases support setting time zones. And other test do it.
And other tests have done the same

There are three distinct data types in Calcite:
TIMESTAMP
TIMESTAMP WITH LOCAL TIME ZONE
TIMESTAMP WITH TIME ZONE

You have to specify the type these functions operate on. I think you probably want functions for each data type. The ones for TIMESTAMP and TIMESTAMP WITH TIMEZONE should be deterministic. And the ones for LOCAL TIME ZONE should be implemented in terms of the other ones by setting the timezone first. But you should not rely on the Java system timezone for this.

Yes, this require more testing, including for BC dates.

mihaibudiu · 2024-08-29T17:59:12Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+//      f.checkString("date_format('1999-13-01', '%X %V')",
+//          null,


what you need is to use an implicit cast to timestamp if the input is a CHAR data type.
you should be able to support that.

mihaibudiu · 2024-08-29T18:00:09Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+          "VARCHAR NOT NULL");
+      f.checkString("date_format(timestamp '1997-10-04 22:23:00', '%V %v %X %x %%')",
+          "39 40 1997 1997 %",
+          "VARCHAR NOT NULL");


can you add some tests with dates before the Gregorian calendar?
I found that some of these fail.
Also, can you say how you validated the outputs?
Are these tested in MySQL?

It throws an exception:Illegal TIMESTAMP literal, In the timestamp scenario, this is consistent with mysql's result.
Yes, I do these tests in mysql . In fact, I write these test cases in mysql and get the results, and then add them to test to validate

The Gregorian Calendar starts in 1582: https://en.wikipedia.org/wiki/Gregorian_calendar
I don't expect that mysql fails in this case.
Perhaps it fails for dates BC.

caicancai · 2024-08-30T02:33:31Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+
+    try {
+      Locale.setDefault(Locale.US);
+      f.checkString("date_format(timestamp '2009-10-04 22:23:00', '%W %M %Y')",


Does 0000-01-01 or BC date support format?

I tested this example in mysql.
SELECT DATE_FORMAT('0000-01-01', '%X %V');
return
0000-01
but
SELECT DATE_FORMAT(timestamp '0000-01-01', '%X %V');
return error: Incorrect DATETIME value: '0000-01-01'

It throws an exception:Illegal TIMESTAMP literal, In the timestamp scenario, this is consistent with mysql's result.

In the Calcite test, this throws an exception because it can't be converted to a timestamp

Yes, Calcite has its own date standard.

SELECT DATE_FORMAT(timestamp '0000-01-01', '%X %V');

This failed because '0000-01-01' is not a timestamp. You can try:

SELECT DATE_FORMAT(data '0000-01-01', '%X %V');

SELECT DATE_FORMAT(timestamp '0000-01-01', '%X %V');

This failed because '0000-01-01' is not a timestamp. You can try:

SELECT DATE_FORMAT(data '0000-01-01', '%X %V');

0000-01-01 is not even a valid date according to the SQL standard. 0000-01-01 is not even a valid date according to the SQL standard. It is difficult to implement the date_format standard in all dialects. Maybe we need to make some decisions

sonarqubecloud · 2024-08-30T16:18:00Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

NobiGo · 2024-08-30T23:45:39Z

site/_docs/reference.md

@@ -2944,6 +2944,7 @@ In the following:
 | b | TIME_SUB(time, interval)                       | Returns the TIME value that is *interval* before *time*
 | b | TIME_TRUNC(time, timeUnit)                     | Truncates *time* to the granularity of *timeUnit*, rounding to the beginning of the unit
 | m o p r | TO_CHAR(timestamp, format)               | Converts *timestamp* to a string using the format *format*
+| m | DATE_FORMAT(timestamp, format)                 | Converts *timestamp* to a string using the format *format*


Do DATE-FORMAT and TO_CHAR have the same functional description? Are their functions confirmed to be consistent？

Yeah, I think they're the same. In fact, Mysql does not have a to_char function.

{@code TO_CHAR} is not supported in MySQL, but it is supported in * MariaDB, a variant of MySQL covered by {@link SqlLibrary#MYSQL}.

NobiGo · 2024-08-30T23:53:02Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

@@ -4919,7 +4920,8 @@ void testBitGetFunc(SqlOperatorFixture f, String functionName) {
  }

  @Test void testToCharPg() {
-    final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.POSTGRESQL);


We check the results by CalciteSqlOperatorTest. In SqlOperatorTest we check the return type.

NobiGo · 2024-08-31T00:03:30Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+
+    try {
+      Locale.setDefault(Locale.US);
+      f.checkString("date_format(timestamp '2009-10-04 22:23:00', '%W %M %Y')",


SELECT DATE_FORMAT(timestamp '0000-01-01', '%X %V');

This failed because '0000-01-01' is not a timestamp. You can try:

SELECT DATE_FORMAT(data '0000-01-01', '%X %V');

NobiGo · 2024-08-31T00:05:49Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+      f.checkString("date_format(timestamp '1997-10-04 22:23:00', '%H %k %I %r %T %S %w')",
+          "22 22 10 10:23:00 PM 22:23:00 00 6",
+          "VARCHAR NOT NULL");
+      f.checkString("date_format(timestamp '1999-01-01', '%X %V')",


In MySQL,This will throw exception ERROR 1525 (HY000) at line 1: Incorrect DATETIME value: '1994-01-01'

Yes, I removed time to simplify testing Case, and in fact, Mysql doesn't support doing that. I'll add time to this test, but for this current test, I'm not sure we can be consistent with Mysql in Calcite

We check the results by CalciteSqlOperatorTest. In SqlOperatorTest we check the return type.

It looks like CalciteSqlOperatorTest is better suited for this test, do I need to modify this test into CalciteSqlOperatorTest as this looks like it needs result checking too

github-actions · 2024-12-05T03:42:18Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

suibianwanwank commented Aug 28, 2024

View reviewed changes

[CALCITE-6551] Add DATE_FORMAT function (enabled in MySQL library)

3ee4432

suibianwanwank force-pushed the mysql_parse_function branch from a9118e5 to 3ee4432 Compare August 28, 2024 17:26

[CALCITE-6551] Pass Test

caf5cf4

mihaibudiu reviewed Aug 29, 2024

View reviewed changes

caicancai reviewed Aug 30, 2024

View reviewed changes

[CALCITE-6551] Review

ad79a4c

NobiGo reviewed Aug 31, 2024

View reviewed changes

suibianwanwank marked this pull request as draft September 3, 2024 11:11

github-actions bot added the stale label Dec 5, 2024

		// f.checkString("date_format('1999-13-01', '%X %V')",
		// null,

[CALCITE-6551] Add DATE_FORMAT function (enabled in MySQL library) #3936

Are you sure you want to change the base?

[CALCITE-6551] Add DATE_FORMAT function (enabled in MySQL library) #3936

Conversation

suibianwanwank commented Aug 28, 2024

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

suibianwanwank commented Aug 28, 2024

mihaibudiu commented Aug 28, 2024

asolimando commented Aug 28, 2024

suibianwanwank commented Aug 29, 2024

mihaibudiu commented Aug 29, 2024

suibianwanwank commented Aug 29, 2024

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

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

caicancai Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 30, 2024

Quality Gate passed

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

suibianwanwank Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2024

caicancai Aug 31, 2024 •

edited

Loading

suibianwanwank Aug 31, 2024 •

edited

Loading