Skip to content

Conversation

kgeis
Copy link
Contributor

@kgeis kgeis commented Jun 5, 2025

This is a followup to #142 to make the integration test match the original code change.

…gresDialect change

The expected error message for timestamp precision range needs to match the range in PostgresDialect.
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kgeis Thanks for the quick fix!

LGTM assuming the CI is green.

Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still failed.

https://github.com/apache/flink-connector-jdbc/actions/runs/15474420512/job/43567756737?pr=172#step:16:1118

Expecting throwable message:
  "The precision of field 'f0' is out of the TIMESTAMP precision range [0, 6] supported by CrateDB dialect."
to contain:
  "The precision of field 'f0' is out of the TIMESTAMP precision range [1, 6] supported by CrateDB dialect."

@kgeis
Copy link
Contributor Author

kgeis commented Jun 6, 2025

Sorry, I didn't realize that change would have impacts elsewhere.

Reviewing the code, I question the decision to have the CrateDB dialect extend the PostgreSQL. Just because they talk the same wire protocol doesn't mean their SQL or data types will be the same. For example, CREATE TABLE T (f0 TIMESTAMP(3)) works on PostgreSQL 17 but CrateDB 5.10 complains

Error!

SQLParseException[The 'timestamp' type doesn't support type parameters.]

That said, I was able to get the tests running locally (at least through the CrateDB tests), and when I updated the CrateDBDialectTest to follow the PostgreSQLDialectTest, they passed.

@kgeis kgeis requested a review from 1996fanrui June 6, 2025 04:29
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @matriv @eskabetxe , would you mind double checking this PR as well? Thanks in advance.

The background could be found from #142 and FLINK-36303

Copy link
Member

@eskabetxe eskabetxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with the change as we are fixing on postgres and not introducing nothing new on CrateDB

We could check after (maybe in another Jira) the CrateDB extension, for timestamp we should fix this, as its only allowed timestmap, the precision should be 3 on min and max.

Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kgeis for the update, and @eskabetxe for the review, merging

@1996fanrui 1996fanrui merged commit 4d3351e into apache:main Jun 12, 2025
4 checks passed
Copy link

boring-cyborg bot commented Jun 12, 2025

Awesome work, congrats on your first merged pull request!

Biasqo pushed a commit to Biasqo/flink-connector-jdbc that referenced this pull request Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants