[CALCITE-6226] Wrong ISOWEEK and no ISOYEAR on BigQuery FORMAT_DATE by rorueda · Pull Request #3653 · apache/calcite

rorueda · 2024-01-26T12:29:18Z

https://issues.apache.org/jira/browse/CALCITE-6226

JiajunBernoulli · 2024-01-26T12:45:20Z

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

    }
  },
-  D("F", "The weekday (Monday as the first day of the week) as a decimal number (1-7)") {
+  // TODO: Parsing of weekdays with sunday as first day of the week


Why use Todo?
Can we finish it in the ticket?

I don't think we can. I'm not that familiar with the codebase, but my understanding is that the parsing uses java's DateFormat class that that simply doesn't have a pattern for the weekday with Sunday as first day of the week.

There are other TODOs related to parsing in this class, and I think to make it all work a more broad/complex change is needed, mainly because in some cases not only the pattern is important when parsing, but also the calendar.

Note that I only removed the "F" because it is not the pattern for weekday. I know the best would be to fix it all, but I think it's better to point out it's not implemented yet than to have an implementation that gives us the wrong result.

Please remove the TODO, log a jira case.

Created CALCITE-6233 to handle the parsing of ISOWEEK.

It doesn't seem that parsing the weekday with Sunday as first day (1) is possible with java's DateFormat.

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

tanclary · 2024-01-26T18:19:42Z

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

+      final Calendar calendar = Work.get().calendar;
+      calendar.setTime(date);
+      int weekDay = calendar.get(Calendar.DAY_OF_WEEK);
+      sb.append(String.format(Locale.ROOT, "%d", weekDay == 1 ? 7 : weekDay - 1));


Does this just convert from Sunday being 1 to Monday being 1?

Yes. Maybe change the comparison to weekDay == Calendar.SUNDAY to make it clearer? Do you see another way?

That could work, or a simple comment could help also. It is no big deal either way :)

In my mind Calcite is complex enough that when we have opportunities to explain little pieces of logic to reduce mental load for the developer, we should

The DOW function implementation assumes that Sunday = 1.
In Postgres DOW assumes that Sunday = 0.

Added a comment - don't think my suggestion would make it clearer.

Looking at Postgres documentation, the EXTRACT(DOW... assumes Sunday = 0, but the format functions to_char, to_date,... also assume Sunday = 1 for day of week.

julianhyde · 2024-01-29T20:02:31Z

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

    }
  },
-  D("F", "The weekday (Monday as the first day of the week) as a decimal number (1-7)") {
+  // TODO: Parsing of weekdays with sunday as first day of the week


Please remove the TODO, log a jira case.

julianhyde · 2024-01-29T20:06:15Z

core/src/test/java/org/apache/calcite/util/format/FormatElementEnumTest.java

-    assertFormatElement(FormatElementEnum.IW, "2014-09-30T10:00:00Z", "40");
+  @Test void testID() {
+    assertFormatElement(FormatElementEnum.ID, "2014-09-30T10:00:00Z", "2");
+  }


I don't understand why some of the tests in this class are parameterized and others are not.

Frankly I don't think the framework is helping make the tests more understandable. I would move to code, and add comments if a test is testing something subtle.

It seems to me that this test class should be very simple.

As there is one method for each format element, I didn't want to add new methods to test the edge cases of ISOWEEK and ISOYEAR, and I thought a good solution was to make those tests parameterized.

But I agree that the way it was done didn't help in making the added test cases understandable. I removed the parameters and made more assertions inside the method (with a comment).

sonarqubecloud · 2024-01-30T12:04:48Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
83.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mihaibudiu · 2024-03-08T21:36:50Z

core/src/test/java/org/apache/calcite/util/format/FormatElementEnumTest.java

+  @Test
+  void testIW() {
    assertFormatElement(FormatElementEnum.IW, "2014-09-30T10:00:00Z", "40");
+    // edge case where ISO WEEK != WEEK


I have discovered that the Java Calendar class doesn't handle correctly some operations with dates before the Gregorian calendar introduction. Can you please add some tests for dates in that range?
https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-6252
Can you also confirm that the values have been validated against BigQuery?

I added additional test cases, all manually validated against BigQuery.

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

rorueda · 2024-03-09T16:03:11Z

Since I had to rebase it onto master to address the last change request, I squashed the commits.

julianhyde · 2024-03-09T18:03:06Z

It always struck me as strange that this change is using Calendar. Day of the week is simple if you convert to Julian date. JulianOn Mar 9, 2024, at 07:59, rorueda ***@***.***> wrote: @rorueda commented on this pull request. In core/src/test/java/org/apache/calcite/util/format/FormatElementEnumTest.java:

assertFormatElement(FormatElementEnum.IW, "2014-09-30T10:00:00Z", "40");

+ // edge case where ISO WEEK != WEEK I added additional test cases, all manually validated against BigQuery. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

mihaibudiu

I trust that the results were all compared against the reference databases, I didn't check myself.

sonarqubecloud · 2024-03-09T19:51:22Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarCloud

The ISOWEEK format function was not setting the minimalDaysInFirstWeek to 4. To avoid having to set the calendar week definition fields in each format function, a new calendar instance configured with the iso8601 settings was added. The format elements for the ISOYEAR with the century (%G) and without it (%g) were added. Also, the weekday with Monday as first day of week (%u) was fixed.

rorueda · 2024-09-17T12:21:33Z

This is a bit old, so there were conflicts with the current master. I did a rebase and fixed the conflicts.

sonarqubecloud · 2024-09-17T12:41:00Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.4% Coverage on New Code
2.4% Duplication on New Code

See analysis details on SonarCloud

rubenada · 2024-09-18T08:35:44Z

LGTM

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

mihaibudiu · 2024-09-20T21:15:42Z

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

          "VARCHAR NOT NULL");
      f.checkString("to_char(timestamp '2022-06-03 13:15:48.678', 'IW')",
-          "23",
+          "22",


this was a bug?

Yes, same ISOWEEK issue, but for PostgreSQL: #3653 (comment)

JiajunBernoulli reviewed Jan 26, 2024

View reviewed changes

tanclary reviewed Jan 26, 2024

View reviewed changes

julianhyde requested changes Jan 29, 2024

View reviewed changes

mihaibudiu reviewed Mar 8, 2024

View reviewed changes

rorueda force-pushed the fix-isoweek branch from 35c428b to abdf4c1 Compare March 9, 2024 15:56

mihaibudiu approved these changes Mar 9, 2024

View reviewed changes

jduo approved these changes Mar 18, 2024

View reviewed changes

rorueda force-pushed the fix-isoweek branch from e141648 to 71b5c96 Compare September 17, 2024 12:10

rubenada added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 18, 2024

mihaibudiu reviewed Sep 20, 2024

View reviewed changes

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

mihaibudiu approved these changes Sep 20, 2024

View reviewed changes

rubenada merged commit 8ffcc2a into apache:main Sep 23, 2024

rorueda deleted the fix-isoweek branch June 4, 2025 13:28

Conversation

rorueda commented Jan 26, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanclary Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sonarqubecloud bot commented Jan 30, 2024

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rorueda commented Mar 9, 2024

Uh oh!

julianhyde commented Mar 9, 2024 via email

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 9, 2024

Quality Gate passed

Uh oh!

rorueda commented Sep 17, 2024

Uh oh!

sonarqubecloud bot commented Sep 17, 2024

Quality Gate passed

Uh oh!

rubenada commented Sep 18, 2024

Uh oh!

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!

7 participants

tanclary Jan 26, 2024 •

edited

Loading