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

[normanj-bitquill]

• 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

[normanj-bitquill]

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

[mihaibudiu]

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]

@mihaibudiu

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?

[mihaibudiu]

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

[mihaibudiu]

I have approved, but also left some questions.

[mihaibudiu]

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).

[normanj-bitquill]

@caicancai

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

[normanj-bitquill]

@mihaibudiu

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.

[normanj-bitquill]

@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

[mihaibudiu]

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...

[normanj-bitquill]

@mihaibudiu

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]

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.

[mihaibudiu]

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]

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)".

[normanj-bitquill]

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.

[normanj-bitquill]

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'.

[normanj-bitquill]

@mihaibudiu I tried creating a new PR here, that did build correctly. https://github.com/apache/calcite/pull/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

[sonarqubecloud[bot]]

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

[mihaibudiu]

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.

[mihaibudiu]

Closing in favor of #3792