New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CALCITE-6358] Support all PostgreSQL 14 date/time patterns #3773

Open

normanj-bitquill wants to merge 6 commits into apache:main from Bit-Quill:calcite-6358

Contributor

normanj-bitquill commented Apr 25, 2024

Splits the PostgreSQL toChar function off to its own function
Does not implement FX or TM prefix
Does not implement SP suffix
Timezone patterns are supported but all datetimes are in local timezone

Contributor Author

normanj-bitquill commented Apr 25, 2024

Need to properly separate the MySQL/Oracle implementation of TO_CHAR from the PostgreSQL implementation.

Contributor

mihaibudiu commented Apr 25, 2024

The amount of testing is very impressive. These won't be easy to review. I assume they were checked against postgres.
You should address the failures reported by the CI.

normanj-bitquill force-pushed the calcite-6358 branch from 8ec9cf8 to 54a2004 Compare

April 25, 2024 19:50

Contributor Author

normanj-bitquill commented Apr 26, 2024

The amount of testing is very impressive. These won't be easy to review. I assume they were checked against postgres. You should address the failures reported by the CI.

These were tested against PostgreSQL 14. The CI issues have been addressed.

Is there anything that I can be done to make reviewing this easier?

Contributor

mihaibudiu commented Apr 26, 2024

The statement that they were run against Postgres helps a lot. Thanks!

mihaibudiu approved these changes

View reviewed changes

babel/src/test/resources/sql/postgresql.iq

    
              VI

              !ok

              select to_char(timestamp '2022-06-03 13:15:48.678', 'rm');

Contributor

mihaibudiu Apr 26, 2024

can we have a day of the week test for a date before the gregorian calendar?
I noticed in the past that these could be wrong.

Contributor Author

normanj-bitquill Apr 29, 2024

There is a test for this on line 316. It tests Jan 1 of year 1 to see if it is a Monday.

core/src/test/java/org/apache/calcite/util/format/PostgresqlDateTimeFormatterTest.java Outdated

    
                  final Locale originalLocale = Locale.getDefault();

                  try {

                    Locale.setDefault(Locale.US);

                    assertEquals("   MONDAY", PostgresqlDateTimeFormatter.toChar("DAY", date1));

Contributor

mihaibudiu Apr 26, 2024

what is the rule about leading spaces?

Contributor Author

normanj-bitquill Apr 29, 2024

From this table:
https://www.postgresql.org/docs/14/functions-formatting.html#FUNCTIONS-FORMATTING-DATETIME-TABLE

it says that days are blank padded to 9 characters.

Contributor Author

normanj-bitquill Apr 29, 2024

The padding should be at the end. I'll update.

Contributor Author

normanj-bitquill Apr 29, 2024

Updated.

core/src/main/java/org/apache/calcite/util/format/PostgresqlDateTimeFormatter.java Outdated

    
                private interface FormatPattern {

                  /**

                   * Checks if the current position in the format string applies to this format

                   * element. Will produce a formatted string if matched that is the format element

Contributor

mihaibudiu Apr 28, 2024

I find the second phrase difficult to parse,maybe you can rephrase it.

Contributor Author

normanj-bitquill Apr 29, 2024

I've tried to reword this.

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

    
                        "a.m.", "p.m."),

                    new NumberFormatPattern(dt -> {

                      final String formattedYear = String.format(Locale.ROOT, "%0,4d", dt.getYear());

                      if (formattedYear.length() == 4 && formattedYear.charAt(0) == '0') {

Contributor

mihaibudiu Apr 28, 2024

this implies that years above 10000 wont' be correct. Maybe that's within spec.

Contributor Author

normanj-bitquill Apr 29, 2024

%0,4d means to print out at least 4 digits and formatted with commas. A value like 12345 will become 12,345.

There is the special case, where we get a 4 digit string back that has at least one leading 0. In this case, Java will not add the comma. For example 1 will become 0001. Need to insert the comma to match PostgreSQL behaviour.

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

    
                          return yearString.substring(yearString.length() - 1);

                        },

                        "I"),

                    new StringFormatPattern(

Contributor

mihaibudiu Apr 28, 2024

I am not sure Calcite can handle dates BC, but maybe this code should not concern itself with that point.
But you may not be able to test this codepath end-to-end.

core/src/main/java/org/apache/calcite/util/format/PostgresqlDateTimeFormatter.java Outdated

    
                        dt -> {

                          final String monthName =

                              dt.getMonth().getDisplayName(TextStyle.FULL,

                              Locale.getDefault());

Contributor

mihaibudiu Apr 28, 2024

Are you sure about the Locale?
I thought that the spec requires the month name to be in English.

Contributor Author

normanj-bitquill Apr 29, 2024

I see what you mean. It should default to English and I should only translate when the TM prefix is provided. I'll make the changes.

https://www.postgresql.org/docs/14/functions-formatting.html#FUNCTIONS-FORMATTING-DATETIME-TABLE

Contributor Author

normanj-bitquill Apr 29, 2024

Updated to make use of the TM flag.

core/src/main/java/org/apache/calcite/util/format/PostgresqlDateTimeFormatter.java Outdated

    
                        dt -> {

                          final String monthName =

                              dt.getMonth().getDisplayName(TextStyle.SHORT,

                              Locale.ROOT);

Contributor

mihaibudiu Apr 28, 2024

Similar question

Contributor Author

normanj-bitquill Apr 29, 2024

See my answer above. I'll make the changes.

Contributor Author

normanj-bitquill Apr 29, 2024

Updated to make use of the TM flag.

Contributor

mihaibudiu commented Apr 28, 2024

I have approved, but also left some questions.

caicancai reviewed

View reviewed changes

Member

caicancai left a comment

I remember that calcite currently has support for the postgres to_char function. Should we remove the previous support?

normanj-bitquill force-pushed the calcite-6358 branch from 1d02fc0 to 6d69b71 Compare

April 29, 2024 21:22

Contributor

mihaibudiu commented Apr 29, 2024

Until https://issues.apache.org/jira/browse/CALCITE-6281 has a solution, the best way to test this may be to create a Quidem test file (.iq suffix). If that file is identical with the Postgres shell output, you're done. (I have no idea whether the quidem output is compatible with Postgres, though).

Contributor Author

normanj-bitquill commented Apr 29, 2024

I remember that calcite currently has support for the postgres to_char function. Should we remove the previous support?

The existing TO_CHAR function is used by MySQL and Oracle, so it is left in here. This PR adds a new implementation that only PostgreSQL uses.

https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java#L1654

Contributor Author

normanj-bitquill commented Apr 29, 2024

Until https://issues.apache.org/jira/browse/CALCITE-6281 has a solution, the best way to test this may be to create a Quidem test file (.iq suffix). If that file is identical with the Postgres shell output, you're done. (I have no idea whether the quidem output is compatible with Postgres, though).

Thanks, I'll take a look.

Contributor Author

normanj-bitquill commented Apr 30, 2024

@mihaibudiu Here is the output of a PostgreSQL script running all of the TO_CHAR tests from the postgresql.iq file in this PR. I kept the expected output for Calcite in SQL comments to make comparison easier.

This is the SQL script.
to_char.txt

This is the output when running it on PostgreSQL 14.
to_char_results.txt

Contributor

mihaibudiu commented Apr 30, 2024

This is pretty close, the existing file core/src/test/resources/sql/sub-query.iq already uses the postgres format.
What I would do is to make a small python script to convert the postgres output into a .iq file, which you can hopefully just add to the resources directory in java. Then, before you commit, make sure the tests in the file are being executed by introducing a typo. And please add a comment to the iq file stating the Postgres version that was used to generate it. I am not sure whether there is some place where the python script itself could be committed...

Contributor Author

normanj-bitquill commented May 1, 2024

This is pretty close, the existing file core/src/test/resources/sql/sub-query.iq already uses the postgres format. What I would do is to make a small python script to convert the postgres output into a .iq file, which you can hopefully just add to the resources directory in java. Then, before you commit, make sure the tests in the file are being executed by introducing a typo. And please add a comment to the iq file stating the Postgres version that was used to generate it. I am not sure whether there is some place where the python script itself could be committed...

Added the queries here:
https://github.com/Bit-Quill/calcite/blob/calcite-6358/core/src/test/resources/pg_to_char_queries.sql

Here is a script that is able to generate the *.iq file:
https://github.com/Bit-Quill/calcite/blob/calcite-6358/core/src/test/resources/to_char_generate_iq.py

Ran the :core:test target with the generated *.iq file. Verified that the file is run by adding an error. The generated file is executed successfully with no errors.

mihaibudiu added the LGTM-will-merge-soon label

Contributor

mihaibudiu commented May 1, 2024

This looks very good, when they unfreeze the main branch we can merge this.
Regarding the script, it's good for this particular use case, but I am not sure whether it will generalize for arbitrary queries. But we can take this discussion to the other JIRA ticket (the preferred way to design solutions in Calcite is to discuss in JIRA). Some Calcite-isms leak into the iq file, like $EXPR0, so a true solution would probably have a different architecture.

caicancai approved these changes

View reviewed changes

Member

caicancai left a comment

LGTM

normanj-bitquill force-pushed the calcite-6358 branch 2 times, most recently from 23fa524 to 4f2459b Compare

May 10, 2024 16:14

normanj-bitquill mentioned this pull request

[CALCITE-6392] Support all PostgreSQL 14 date/time patterns for to_date/to_timestamp #3791

Merged

arkanovicz reviewed

View reviewed changes

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java Outdated

+                @LibraryOperator(libraries = {POSTGRESQL})
+                public static final SqlFunction TO_CHAR_PG =
+                    new SqlBasicFunction("TO_CHAR", SqlKind.OTHER_FUNCTION,
+                    SqlSyntax.FUNCTION, true, ReturnTypes.VARCHAR, null,

Contributor

arkanovicz May 12, 2024

Why not ReturnTypes.VARCHAR_NULLABLE here? The original Calcite TO_CHAR function result should also be nullable, by the way.

Contributor Author

normanj-bitquill May 13, 2024

Good point, this has been updated.

normanj-bitquill force-pushed the calcite-6358 branch from 4f2459b to b0a732d Compare

May 13, 2024 17:27

Contributor

mihaibudiu commented May 14, 2024

I don't understand why the CI fails.
Can you please rebase on main? Maybe it has something to do with the recent transition to 1.37 which had some build system changes.

normanj-bitquill force-pushed the calcite-6358 branch from b0a732d to d1cfc2a Compare

May 14, 2024 15:23

Contributor Author

normanj-bitquill commented May 14, 2024

I don't understand why the CI fails. Can you please rebase on main? Maybe it has something to do with the recent transition to 1.37 which had some build system changes.

@mihaibudiu I have rebased to the latest for main. CI still fails for "Windows (JDK 8)" and "Windows (JDK 17)".


          [CALCITE-6358] Support all PostgreSQL 14 date/time patterns for to_char

3e87490

* Splits the PostgreSQL toChar function off to its own function
* Does not implement SP suffix
* Timezone patterns are supported but all datetimes are in local timezone

normanj-bitquill force-pushed the calcite-6358 branch from d1cfc2a to 3e87490 Compare

May 14, 2024 18:28

normanj-bitquill added 2 commits

May 14, 2024 13:58


          Increase logging for Windows JDK 8 build

1d64869


          Bumped autostyle to 3.2

421ed42

normanj-bitquill added 2 commits

May 14, 2024 14:42


          Revert back to autostyle 3.0 and enable debug logging

77a9300


          Changed back to info logging

c1234d1

Contributor Author

normanj-bitquill commented May 14, 2024

I have no idea about the Windows build errors. It fails trying to do task :buildSrc:autostyleKotlinGradleCheck. It fails since it can't find the class org.jetbrains.kotlin.com.intellij.psi.tree.IElementType.

I don't see why that is missing due to the changes for this PR.

Contributor Author

normanj-bitquill commented May 14, 2024

Locally with a fresh checkout I get:

Skipping task ':buildSrc:autostyleKotlinGradleCheck' as it is up-to-date.

For the Windows build, I get:

:buildSrc:autostyleKotlinGradleCheck (Thread[Execution worker,5,main]) started.

> Task :buildSrc:autostyleKotlinGradleCheck FAILED
Caching disabled for task ':buildSrc:autostyleKotlinGradleCheck' because:
  Caching has not been enabled for the task
Task ':buildSrc:autostyleKotlinGradleCheck' is not up-to-date because:
  No history is available.
The input changes require a full rebuild for incremental task ':buildSrc:autostyleKotlinGradleCheck'.


          Testing with removing the to_char python script

98fe454

Contributor Author

normanj-bitquill commented May 15, 2024

@mihaibudiu I tried creating a new PR here, that did build correctly.
#3792

If you are OK with it, we should just use that PR and close this one.

I think that there are a couple of things going on.

I had squashed commits, which could be interfering with how changes are detected in autostyle. Lesson learned, I won't squash commits.
Some Kotlin libraries are missing from the classpath for autostyle. Not an issue unless some changes are detected to a Kotlin file. The issue here may show up again if someone makes changes to build.gradle.kts

normanj-bitquill mentioned this pull request

[CALCITE-6358] Support all PostgreSQL 14 date/time patterns for to_char #3792

Open

sonarcloud bot commented May 15, 2024

Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.2% Coverage on New Code
6.6% Duplication on New Code

See analysis details on SonarCloud

Contributor

mihaibudiu commented May 15, 2024

Anything that works correctly is fine for me. Please note that I am on vacation so I won't have time to review this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment