-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[DRAFT] Add support for TIMESTAMP types in Exasol connector #26633
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
[DRAFT] Add support for TIMESTAMP types in Exasol connector #26633
Conversation
17207a2
to
fbab6f8
Compare
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.
improved PR, with more simple and less error-prone solution.
The previous PR is moved to DRAFT, please discard it: #26259
Please, review the new PR, @ebyhr , @chenjian2664 ! 🙏
After reviewing the previous implementation, I believe the previous approach - mapping Exasol TIMESTAMP WITH LOCAL TIME ZONE to Trino TIMESTAMP WITH TIME ZONE - adds unnecessary complexity and potential pitfalls.
Exasol TIMESTAMP WITH LOCAL TIME ZONE does not actually persist any time zone information. Apart from some minor edge cases, its semantics is very close to a plain Exasol TIMESTAMP.
Mapping to Trino TIMESTAMP WITH TIME ZONE requires hardcoding the JVM time zone, which introduces fragility. For example, values valid in the Exasol DB time zone but invalid in the JVM time zone (e.g., during DST gaps) may cause Trino to throw exceptions, potentially breaking queries.
A simpler mapping to Trino TIMESTAMP avoids these problems, is more efficient, and aligns better with how Exasol handles this type internally.
Given this, I created a new PR proposing the TIMESTAMP WITH LOCAL TIME ZONE → TIMESTAMP mapping, which I believe will result in a cleaner, simpler and more reliable solution.
fbab6f8
to
f28a06f
Compare
f28a06f
to
8f6e43c
Compare
plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolClient.java
Show resolved
Hide resolved
plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-exasol/src/test/java/io/trino/plugin/exasol/TestExasolTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-exasol/src/test/java/io/trino/plugin/exasol/TestExasolTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-exasol/src/test/java/io/trino/plugin/exasol/TestExasolTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-exasol/src/test/java/io/trino/plugin/exasol/TestExasolTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-exasol/src/test/java/io/trino/plugin/exasol/TestExasolTypeMapping.java
Outdated
Show resolved
Hide resolved
8f6e43c
to
e0bd02e
Compare
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.
Thank you very much for your review, @ebyhr ! 👍
The changes have been applied.
plugin/trino-exasol/src/test/java/io/trino/plugin/exasol/TestExasolTypeMapping.java
Show resolved
Hide resolved
e0bd02e
to
8c9bdc7
Compare
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
return Optional.of(dateColumnMapping()); | ||
case Types.TIMESTAMP: | ||
case EXASOL_TIMESTAMP_WITH_TIMEZONE: | ||
return Optional.of(timestampColumnMapping(typeHandle)); |
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.
The https://docs.exasol.com/db/latest/sql_references/data_types/datatypedetails.htm#Dateandtimedatatypes shows that timestamp with local time zone type actually has timezone concept, but just store as UTC.
How about support only TIMESTAMP type first in this single pr?
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.
The https://docs.exasol.com/db/latest/sql_references/data_types/datatypedetails.htm#Dateandtimedatatypes shows that timestamp with local time zone type actually has timezone concept, but just store as UTC.
Yes, it has some differences in the time zone behavior, than usual TIMESTAMP, which are reflected in the tests, but when we get the actual column value with trino connector reader we don't know, in which time zone it was saved. So if we map to Trino "Timestamp With Time Zone" we must provide some time zone, which could be hardcoded UTC or JVM Time Zone. But this approach is more fragile and in some extreme cases like DLT, could even provide incorrect mapping. So the solution of mapping to Trino Timestamp is more simple and less error-prone.
How about support only TIMESTAMP type first in this single pr?
The implementations are actually very similar and the second PR would be a change of just two lines of code in the main source and one line in documentation (the tests are also very similar for each type).
This is due to a more simple solution, which maps to Trino Timestamp for both Exasol Types.
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.
Thank you very much for your review, @chenjian2664 👍
The new PR, which contains only TIMESTAMP changes, has been created:
#26963
I will close this PR
Closed as Draft. |
Description
Improved version of the previous PR: #26259
Added Exasol Trino connector support for Timestamp and Timestamp With Local Time Zone JDBC data types
Additional context and related issues
Release notes