[CALCITE-6185] Support date format by caicancai · Pull Request #3611 · apache/calcite

caicancai · 2024-01-04T15:40:27Z

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

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

tanclary

If you rebase on main it might be easier to tell what exactly is being added in this PR (at least for someone like me who reviews based on "Files changed").

caicancai · 2024-01-06T08:00:01Z

I initially wanted to support timezone, but according to my tests, there seems to be no unified standard for timezone.
Don't Merge

Support CC format format and add tests (pg)
Consider supporting timezone,
full lower case day name(day) pg
lower case day name(dy) pg

sonarqubecloud · 2024-01-06T16:33:05Z

Quality Gate passed

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

2 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

XuQianJin-Stars · 2024-01-07T09:49:07Z

Please merge after this pr #3609.

XuQianJin-Stars · 2024-01-07T09:54:22Z

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

 import static org.apache.calcite.util.format.FormatElementEnum.YY;
 import static org.apache.calcite.util.format.FormatElementEnum.YYYY;
+import static org.apache.calcite.util.format.FormatElementEnum.day;
+import static org.apache.calcite.util.format.FormatElementEnum.dy;


The imports here should be in alphabetical order.

I tried it, but it will report an error. It seems that there is a restriction on the upper and lower case order.
import static org.apache.calcite.util.format.FormatElementEnum.*; ?

Wouldn't checkstyle complain if the imports are in the wrong order?

Possibly checkstyle is using a different collation (e.g. different treatment of upper/lower case, spaces, underscores). Do whatever makes checkstyle happy.

Easiest thing is to make the enum constants upper-case.

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

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

tanclary · 2024-01-08T17:21:37Z

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

 * @see FormatModels#DEFAULT
 */
 public enum FormatElementEnum implements FormatElement {
+  CC("cc", "century (2 digits) (the twenty-first century starts on 2001-01-01)") {


I think the commit message should probably be more descriptive, I wasn't sure what Support date format meant until I reviewed the PR.

+1

I won't even review a PR until the jira case has a good description of its goals. Sounds pedantic but it's very pragmatic.

This jira case also needs to say which functions these date formats are supported in. Changes to FormatElementEnum are not useful unless they are surfaced in functions.

tanclary · 2024-01-08T17:23:18Z

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

      sb.append(work.eeeeFormat.format(date));
    }
  },
+  day("eeee", "The full lower case day name") {


Enum constants should be all uppercase I believe.
https://docs.oracle.com/javase/tutorial/java/javaOO/enum.html

tanclary · 2024-01-08T17:23:45Z

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

    assertThat(ts, hasToString("Tuesday"));
  }

+  @Test void testday() {


Test name should be camelcase

tanclary · 2024-01-08T17:23:56Z

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

    assertThat(ts, hasToString("Tue"));
  }

+  @Test void testdy() {


same comment as above

tanclary · 2024-01-08T17:24:27Z

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

  }
+
+  @Test void testCC() {
+    final String date = "2014-09-30T10:00:00Z";


Instead of adding to the end of the file why don't we make these alphabetical

caicancai · 2024-01-09T05:46:17Z

Thanks everyone.
I thought about it and found that my jira is indeed irregular. I am sorry for causing trouble to the community. I will improve my jira later.

caicancai · 2024-01-09T06:24:49Z

I will open this pr after clarifying the overall architecture and code structure of calcite. If I feel that this pr has a pr at that time,

caicancai marked this pull request as draft January 4, 2024 15:40

caicancai force-pushed the 6185 branch from e063405 to bcda815 Compare January 4, 2024 15:46

caicancai closed this Jan 4, 2024

caicancai reopened this Jan 4, 2024

caicancai force-pushed the 6185 branch from bcda815 to 41c35b1 Compare January 4, 2024 15:56

julianhyde reviewed Jan 4, 2024

View reviewed changes

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

julianhyde force-pushed the main branch from 88a4150 to 3c19347 Compare January 4, 2024 20:49

tanclary reviewed Jan 5, 2024

View reviewed changes

caicancai force-pushed the 6185 branch 3 times, most recently from 2c6d68d to b042e5c Compare January 6, 2024 07:58

caicancai changed the title ~~[CALCITE-6185] Support more date format~~ [CALCITE-6185] Support century function format Jan 6, 2024

caicancai requested a review from tanclary January 6, 2024 11:53

caicancai force-pushed the 6185 branch 2 times, most recently from deec335 to 96b7123 Compare January 6, 2024 16:09

caicancai changed the title ~~[CALCITE-6185] Support century function format~~ [CALCITE-6185] Support date format Jan 6, 2024

XuQianJin-Stars reviewed Jan 7, 2024

View reviewed changes

tanclary requested changes Jan 8, 2024

View reviewed changes

caicancai force-pushed the 6185 branch from 96b7123 to 7efef07 Compare January 8, 2024 09:42

[CALCITE-6185] Support more date format

843055b

caicancai force-pushed the 6185 branch from 7efef07 to 843055b Compare January 8, 2024 09:54

caicancai requested review from XuQianJin-Stars and tanclary January 8, 2024 10:04

caicancai marked this pull request as ready for review January 8, 2024 10:45

tanclary requested changes Jan 8, 2024

View reviewed changes

caicancai marked this pull request as draft January 9, 2024 06:00

caicancai closed this Jan 9, 2024

caicancai deleted the 6185 branch February 15, 2024 12:03

Conversation

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

Uh oh!

Uh oh!

Uh oh!

tanclary left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

sonarqubecloud bot commented Jan 6, 2024

Quality Gate passed

Uh oh!

XuQianJin-Stars commented Jan 7, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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

Uh oh!

Uh oh!

caicancai commented Jan 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

caicancai commented Jan 4, 2024 •

edited

Loading

caicancai commented Jan 6, 2024 •

edited

Loading

caicancai commented Jan 9, 2024 •

edited

Loading