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
base: master
Are you sure you want to change the base?
Conversation
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. |
@lopata2 can you add a connection property to enable this ? |
@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. |
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 |
@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. |
checkstyle is failing... |
fixed |
There was a problem hiding this 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:
- Add tests for
ResultSetMetaData
- 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:
pgjdbc/.github/workflows/matrix.js
Line 117 in 7dd20dd
matrix.addAxis({ pgjdbc/.github/workflows/matrix.js
Lines 195 to 199 in 7dd20dd
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', ]); pgjdbc/.github/workflows/main.yml
Line 184 in 7dd20dd
preferQueryMode=${{ matrix.query_mode }}
…ITH_TIMEZONE and not to TIMESTAMP
…NE and not to TIME
…ia connection properties
8ed9370
to
b568702
Compare
SQL_TYPES_WITH_TIMEZONE( | ||
"sqlTypesWithTimezone", | ||
"false", | ||
"Enable or disable mapping of PG types with TIMEZONE into SQL types with TIMEZONE." |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
@vlsi other than the enum style property this is ready for review |
…ITH_TIMEZONE and not to TIMESTAMP
All Submissions:
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.