Skip to content
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.

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

correct mapping for postgres timestamptz type to sql type TIMESTAMP_W… #2715

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

lopata2
Copy link

@lopata2 lopata2 commented Jan 5, 2023

…ITH_TIMEZONE and not to TIMESTAMP

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

PR is solving this the issue described here #1766 .

This PR solves the problem for the applications that use column metadata to find out whether the column is TIMESTAMP or TIMESTAMP WITH TIME ZONE and based on that info they convert the value they want to store.

@vlsi
Copy link
Member

vlsi commented Jan 12, 2023

I guess this changes behaviour, so the only way we can make it is via connection property, so we release a version that adds a property and keeps old behaviour, then keep it for a while, and then release a version that flips the default.

@davecramer
Copy link
Member

@lopata2 can you add a connection property to enable this ?

@lopata2
Copy link
Author

lopata2 commented Jan 26, 2023

@davecramer I will try to do that. The same apply also for the TIME and TIME_WITH_ZONE types. Should I do the same change for that data type in this PR or its better to create new PR.

@davecramer
Copy link
Member

@davecramer I will try to do that. The same apply also for the TIME and TIME_WITH_ZONE types. Should I do the same change for that data type in this PR or its better to create new PR.

You can put them in the same PR.

@davecramer
Copy link
Member

So in order to accept this patch we need to put a configuration parameter to enable it. The default would be to leave the behaviour the same

@lopata2
Copy link
Author

lopata2 commented Jan 31, 2023

@davecramer I have added new PGProperty SQL_TYPES_WITH_TIMEZONE that controls the mapping of PG timezone types. The default value is false and the behavior should be the same as it is now. I have also modified the tests.

@davecramer
Copy link
Member

checkstyle is failing...

@lopata2
Copy link
Author

lopata2 commented Feb 1, 2023

fixed

Copy link
Member

@vlsi vlsi left a comment

Choose a reason for hiding this comment

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

@lopata2 ,

I am afraid we won't be able to merge a PR that changes timestamp behaviour without having a clear case that exercises values of the timestamp in question.

Please:

  1. Add tests for ResultSetMetaData
  2. Add tests for values produced and consumed by the driver in case the property is changed from its default value. For instance, it might be a good idea to add property to the CI matrix:
    matrix.addAxis({
    ,
    matrix.setNamePattern([
    'java_version', 'java_distribution', 'pg_version', 'query_mode', 'scram', 'ssl', 'hash', 'os',
    'server_tz', 'tz', 'locale',
    'check_anorm_sbt', 'gss', 'replication', 'slow_tests',
    ]);
    , and
    preferQueryMode=${{ matrix.query_mode }}

@davecramer davecramer force-pushed the fix-timestamp-with-zone-handling branch from 8ed9370 to b568702 Compare April 23, 2024 20:14
SQL_TYPES_WITH_TIMEZONE(
"sqlTypesWithTimezone",
"false",
"Enable or disable mapping of PG types with TIMEZONE into SQL types with TIMEZONE."
Copy link
Member

Choose a reason for hiding this comment

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

I might be wrong, however, it looks like the description does not explain the meaning of the property.

* The default is {@code false}
*/
SQL_TYPES_WITH_TIMEZONE(
"sqlTypesWithTimezone",
Copy link
Member

Choose a reason for hiding this comment

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

I would say we should select a better name that would be searchable and understandable.

*/
SQL_TYPES_WITH_TIMEZONE(
"sqlTypesWithTimezone",
"false",
Copy link
Member

Choose a reason for hiding this comment

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

We might consider enum style property instead as it might be easier to reason.

Copy link
Member

Choose a reason for hiding this comment

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

@vlsi if we used enum style what would the two names be ?

@davecramer
Copy link
Member

@vlsi other than the enum style property this is ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants