Add support for TIMESTAMP and TIMESTAMP WITH TIME ZONE types in Exasol connector #26259
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
skyglass
force-pushed
the
feature/724_timestamp_data_types
branch
from
37a2fe5 to
78d1495
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
skyglass
changed the title
skyglass
changed the title
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
skyglass
force-pushed
the
feature/724_timestamp_data_types
branch
from
c950205 to
d7ff56f
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
- implemented additional timestamp with precision data type for ExasolClient
- added correspondent tests for timestamp with precision data type
- implemented additional timestamp with local time zone data type for ExasolClient
- implemented special addRoundTrip method with additional column expression parameter for the predicate to fix the test predicate assumption
- all timestamp with timezone tests use input literal timestamp string, which is interpreted as a JVM timestamp string ("America/Bahia_Banderas") but expected literal strings are interpreted as UTC strings. Therefore the difference between these timestamps is 6 hours (with DST) or 5 (without DST); for some historical values, like 1970 for example, the difference can be 7, which is expected behaviour for timestamp with local time zone
- removed mapper classes and moved the logic to ExasolClient, based on the PR suggesions
- removed unnecessary javadocs
- improved javadocs and comments for testing timestamp with time zone by explaining why only jvm time zone is currently used for testing timestamp with time zone
- removed using * import in ExasolClient based on the PR suggestions
- removed TestExasolTimestampMapping and moved testing timestamp and timestamp with timezone to TestExasolTypeMapping, based on the PR suggestions
- defined test methods as package-private, based on the PR suggestions
skyglass
force-pushed
the
feature/724_timestamp_data_types
branch
from
ef6b95a to
07a421f
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| @@ -28,6 +28,8 @@ | |||
| import io.trino.plugin.jdbc.JdbcTypeHandle; | |||
| import io.trino.plugin.jdbc.LongReadFunction; | |||
| import io.trino.plugin.jdbc.LongWriteFunction; | |||
| import io.trino.plugin.jdbc.ObjectReadFunction; | |||
There was a problem hiding this comment.
The 1st commit contains redundant commit body. Please remove it.
| @@ -17,6 +17,7 @@ | |||
| import org.testng.annotations.Test; | |||
|
|
|||
| import java.math.BigDecimal; | |||
| import java.sql.Timestamp; | |||
There was a problem hiding this comment.
- implemented testing of timestamp columns in the product test
I would drop this commit.
Product tests are useful for catching class loader issues. TestExasolTypeMapping is sufficient in this case.
https://trino.io/docs/current/develop/tests.html#avoid-product-tests
| @@ -101,6 +101,13 @@ Trino data type mapping: | |||
| * - ``DATE`` | |||
| - ``DATE`` | |||
| - | |||
| * - ``TIMESTAMP(n)`` | |||
There was a problem hiding this comment.
Updated documentation with new TIMESTAMP type mappings
Please squash this commit into the 1st commit. We don't separate a commit for documentation.
| Exasol `TIMESTAMP(n)` columns are mapped to Trino's `TIMESTAMP(n)` type and vice versa, with the following exceptions: | ||
|
|
||
| - **No precision specified**: | ||
| If the precision is omitted (i.e., the column is defined as `TIMESTAMP` without `(n)`), Exasol defaults to a precision of 3. This maps to Trino's `TIMESTAMP(3)`. |
There was a problem hiding this comment.
Please wrap at 80 characters. Same for other places.
| long nanosUtc = millisUtc * 1_000_000L + timestamp.getNanos() % 1_000_000; | ||
| int picosOfMilli = (int) ((nanosUtc - millisUtc * 1_000_000) * 1_000); |
There was a problem hiding this comment.
There are some constants in Timestamps.
| test.execute(getQueryRunner(), session, exasolCreateAndInsert(TEST_SCHEMA + "." + "test_timestamp")); | ||
| } | ||
|
|
||
| private void testTimestamp(ZoneId sessionZone) |
There was a problem hiding this comment.
Move this method under testTimestamp().
| * This difference is intentional and verifies correct conversion between zones. | ||
| * <p> | ||
| */ | ||
| private void testTimestampWithTimeZone(ZoneId sessionZone) |
There was a problem hiding this comment.
Move this method under testTimestampWithTimeZone().
| } | ||
|
|
||
| @Test | ||
| void testUnsupportedTimestampValues() |
There was a problem hiding this comment.
Could you add a similar test with timestamp with time zone type?
| assertExasolQueryFails( | ||
| "INSERT INTO " + table.getName() + " VALUES (TIMESTAMP '2024-01-01 12:34:56.1234567890')", | ||
| "data exception - invalid character value for cast"); |
There was a problem hiding this comment.
This should round values instead of failures in my opinion.
| void testTimestampWithTimeZone() | ||
| { | ||
| //Only JVM Time Zone is currently used for testing timestamp with time zone | ||
| //Adding test cases for other time zones would require improving the test to forcibly apply session changes for timestamp with timezone |
There was a problem hiding this comment.
I don't understand this comment. Can you elaborate on that?
Description
Added Exasol Trino connector support for Timestamp and Timestamp With Local Time Zone JDBC data types
Additional context and related issues
Release notes