Add support for TIMESTAMP type in exasol connector #26963
+302
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThe PR introduces full support for Exasol TIMESTAMP type in the Trino connector by extending JDBC type handling in ExasolClient with precision-aware read/write functions, adding extensive round-trip and error-handling tests in TestExasolTypeMapping, and updating documentation accordingly. ER diagram for TIMESTAMP type mapping between Exasol and TrinoerDiagram
EXASOL_TIMESTAMP {
precision int
}
TRINO_TIMESTAMP {
precision int
}
EXASOL_TIMESTAMP ||--|| TRINO_TIMESTAMP : "maps to"
Class diagram for updated ExasolClient TIMESTAMP supportclassDiagram
class ExasolClient {
+toColumnMapping(session, typeHandle)
+timestampColumnMapping(typeHandle)
+longTimestampReadFunction(timestampType)
+longTimestampWriteFunction(timestampType)
+objectTimestampReadFunction(timestampType)
+objectTimestampWriteFunction(timestampType)
+verifyObjectTimestampPrecision(timestampType)
+getTimestampBindExpression(precision)
MAX_EXASOL_TIMESTAMP_PRECISION : int
}
class TimestampType {
+getPrecision()
+isShort()
}
class LongTimestamp {}
ExasolClient --> TimestampType
ExasolClient --> LongTimestamp
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In both LongWriteFunction and ObjectWriteFunction setNull methods you’re calling statement.setNull(index, Types.VARCHAR), but since this is a TIMESTAMP column you should use Types.TIMESTAMP to avoid unintended type conversions.
- Instead of hardcoding MAX_EXASOL_TIMESTAMP_PRECISION = 9 in ExasolClient, consider reusing the max precision constant from TimestampType (e.g. TimestampType.MAX_PRECISION) or centralizing it to avoid duplication.
- In testUnsupportedInsertValue you hardcode the table name to test_unsupported_hashtype; switch to a per-test randomNameSuffix (like in testUnsupportedDefinition) to prevent conflicts when running tests in parallel.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both LongWriteFunction and ObjectWriteFunction setNull methods you’re calling statement.setNull(index, Types.VARCHAR), but since this is a TIMESTAMP column you should use Types.TIMESTAMP to avoid unintended type conversions.
- Instead of hardcoding MAX_EXASOL_TIMESTAMP_PRECISION = 9 in ExasolClient, consider reusing the max precision constant from TimestampType (e.g. TimestampType.MAX_PRECISION) or centralizing it to avoid duplication.
- In testUnsupportedInsertValue you hardcode the table name to test_unsupported_hashtype; switch to a per-test randomNameSuffix (like in testUnsupportedDefinition) to prevent conflicts when running tests in parallel.
## Individual Comments
### Comment 1
<location> `plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolClient.java:345` </location>
<code_context>
+ public void setNull(PreparedStatement statement, int index)
+ throws SQLException
+ {
+ statement.setNull(index, Types.VARCHAR);
+ }
+ };
</code_context>
<issue_to_address>
**issue (bug_risk):** Using Types.VARCHAR for setting null on TIMESTAMP columns may be incorrect.
In both longTimestampWriteFunction and objectTimestampWriteFunction, setNull should use Types.TIMESTAMP for TIMESTAMP columns to avoid potential JDBC or Exasol issues. Change to Types.TIMESTAMP unless VARCHAR is required for a specific reason.
</issue_to_address>
### Comment 2
<location> `plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolClient.java:317-318` </location>
<code_context>
+ private static LongReadFunction longTimestampReadFunction(TimestampType timestampType)
+ {
+ return (resultSet, columnIndex) -> {
+ Timestamp timestamp = resultSet.getTimestamp(columnIndex);
+ return toTrinoTimestamp(timestampType, timestamp.toLocalDateTime());
+ };
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Null handling for ResultSet.getTimestamp is missing.
A null check should be added before calling toLocalDateTime() to prevent a NullPointerException when the database value is NULL.
</issue_to_address>
### Comment 3
<location> `plugin/trino-exasol/src/test/java/io/trino/plugin/exasol/TestExasolTypeMapping.java:566-572` </location>
<code_context>
+ expectedException);
+ }
+
+ private void testUnsupportedInsertValue(
+ String exasolType,
+ String inputLiteral,
+ String expectedException)
+ {
+ try (TestTable table = new TestTable(onRemoteDatabase(), TEST_SCHEMA + ".test_unsupported_hashtype", "(col %s)".formatted(exasolType))) {
+ assertExasolSqlQueryFails("INSERT INTO %s VALUES (%s)".formatted(table.getName(), inputLiteral), expectedException);
+ }
+ }
</code_context>
<issue_to_address>
**question (testing):** Question about test table naming for unsupported insert value tests.
Consider parameterizing the test table name or generating a unique name for each test to prevent conflicts during parallel test execution.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolClient.java
Show resolved
Hide resolved
This was referenced Oct 15, 2025
skyglass
force-pushed
the
feature/724_only_timestamp_data_type
branch
from
8dc68e8 to
70b19d8
Compare
MAX_EXASOL_TIMESTAMP_PRECISION is 9, butTimestampType.MAX_PRECISION is 12, so we have to create a separate constant. |
skyglass
force-pushed
the
feature/724_only_timestamp_data_type
branch
from
70b19d8 to
a417a34
Compare
| FULL_PUSHDOWN); | ||
| } | ||
|
|
||
| private static LongReadFunction longTimestampReadFunction(TimestampType timestampType) |
There was a problem hiding this comment.
Please verify the timestamp precision is in the expected range
|
|
||
| private static LongWriteFunction longTimestampWriteFunction(TimestampType timestampType) | ||
| { | ||
| return new LongWriteFunction() |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added Exasol Trino connector support for Timestamp JDBC data type
Additional context and related issues
Release notes
Summary by Sourcery
Add support for the Exasol TIMESTAMP type by mapping it to Trino’s TIMESTAMP(n) and handling precision, read/write functions, and binding; include comprehensive tests for round-trip behavior across time zones (including DST edge cases) and unsupported values, and update connector documentation.
New Features:
Enhancements:
Documentation:
Tests: