DanielLeens commented on PR #11485:
URL: https://github.com/apache/seatunnel/pull/11485#issuecomment-5343030256
Thanks for working on this! I re-derived this review from scratch against
the current head (`9f534f6800ad6f1a476452e56511bb167726734b`), reading the real
diff and tracing the type-mapping call chain rather than relying on the PR
description or prior review notes.
# What Problem Does This PR Solve?
- **User pain point**: Several JDBC dialects (DB2, Presto, Teradata,
Vertica) and the generic/deprecated catalog fallback (`JdbcColumnConverter`)
either threw `UnsupportedOperationException` /
`CommonError.convertToSeaTunnelTypeError`, or silently dropped the UTC offset
by folding into `LOCAL_DATE_TIME_TYPE`, whenever a source table had a
`TIMESTAMP WITH TIME ZONE` / `TIMESTAMPTZ` column. This PR is the tail end of
the `TIMESTAMP_TZ` migration started by PR #10048 (Postgres) and the broader
effort recorded in `incompatible-changes.md`.
- **Fix approach**: Add a `TIMESTAMP_WITH_TIMEZONE → OFFSET_DATE_TIME_TYPE`
case to `GenericTypeConverter` and `DB2TypeConverter`; add native-type-name
recognition to `PrestoTypeMapper`, `TeradataTypeMapper`, `VerticaTypeMapper`;
teach the generic `JdbcColumnConverter` fallback to reclassify a column as
`TIMESTAMP_WITH_TIMEZONE` from its native type name even when the driver's own
`DATA_TYPE` code doesn't say so; and add a DB2-for-z/OS-specific value reader
(`DBTimestamp.toDBString(true)` via reflection) because `ResultSet.getString()`
strips the offset for that driver's proprietary type.
- **One-sentence summary**: Closes out `TIMESTAMP_TZ` support for the
remaining JDBC dialects, but the two dialects with the least mature JDBC
drivers (Teradata, Vertica) get weaker guarantees than Presto/DB2 got in this
same PR, and the branch itself is currently unmergeable against `dev`.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Files touched (production code)**: `JdbcColumnConverter.java`,
`GenericTypeConverter.java`, `DB2TypeConverter.java`, `PrestoDialect.java`,
`PrestoTypeMapper.java`, `TeradataTypeMapper.java`, `VerticaTypeMapper.java`,
`JdbcFieldTypeUtils.java`.
The critical path is schema discovery (`*TypeMapper.mapping(...)` /
`*TypeConverter.convert(...)`, invoked once per column at
catalog/schema-inference time) feeding into per-row value reads
(`AbstractJdbcRowConverter.toInternal()` → `case TIMESTAMP_TZ:
JdbcFieldTypeUtils.getOffsetDateTime(rs, idx)`, unchanged in this PR, at
`AbstractJdbcRowConverter.java:121-125`). If schema discovery gets the type
wrong, either the column is unreadable (fails fast) or the value reader
mis-parses it (silently wrong).
**Before/after — `JdbcColumnConverter.java` (the deprecated generic catalog
fallback, still live — see below):**
```java
// before
switch (jdbcType) {
...
case TIMESTAMP:
case TIMESTAMP_WITH_TIMEZONE:
seaTunnelType = LocalTimeType.LOCAL_DATE_TIME_TYPE;
break;
// after
int effectiveJdbcType =
isTimestampWithTimeZone(nativeType) ? TIMESTAMP_WITH_TIMEZONE :
jdbcType;
switch (effectiveJdbcType) {
...
case TIMESTAMP:
seaTunnelType = LocalTimeType.LOCAL_DATE_TIME_TYPE;
break;
case TIMESTAMP_WITH_TIMEZONE:
seaTunnelType = LocalTimeType.OFFSET_DATE_TIME_TYPE;
break;
```
`isTimestampWithTimeZone` matches `"TIMESTAMPTZ"` exactly or
`TIMESTAMP(?:\s*\(\s*\d+\s*\))?\s+WITH\s+TIME\s+ZONE` against the upper-cased
native type name (`JdbcColumnConverter.java:265-272`).
**Before/after — `GenericTypeConverter.java`:**
```java
// before: no case for Types.TIMESTAMP_WITH_TIMEZONE at all → fell into
`default:` →
// log.warn(...) and the builder's dataType is left unset.
// after:
case Types.TIMESTAMP_WITH_TIMEZONE:
builder.dataType(LocalTimeType.OFFSET_DATE_TIME_TYPE);
builder.scale(typeDefine.getScale());
break;
```
**`PrestoTypeMapper.java` — the interesting fix in this PR:**
```java
String columnType = metadata.getColumnTypeName(colIndex).toUpperCase();
// Presto can report a precision between TIMESTAMP and WITH TIME ZONE.
if (columnType.startsWith(PRESTO_TIMESTAMP) && columnType.contains("WITH
TIME ZONE")) {
columnType = PRESTO_TIMESTAMP_WITH_TIME_ZONE;
}
// VARCHAR(x) ---> VARCHAR
if (columnType.indexOf("(") > -1) {
columnType = columnType.split("\\(")[0];
}
```
This is correct and necessary: Presto/Trino reports `timestamp(3) with time
zone`, and the pre-existing generic `split("\\(")[0]` strip (needed for
`VARCHAR(255)` etc.) would otherwise truncate that string to bare
`"TIMESTAMP"`, losing the `WITH TIME ZONE` suffix and silently dropping the
column into the timezone-naive branch. The normalization runs *before* the
generic paren-strip, so precision-qualified input is handled correctly.
Verified against
`TimestampWithTimeZoneTypeMappingTest.testPrestoTimestampWithTimeZone`.
**`TeradataTypeMapper.java` / `VerticaTypeMapper.java` — the same class of
fix is NOT applied here:**
```java
// TeradataTypeMapper.mapping(), no precision normalization at all:
String teradataType = metadata.getColumnTypeName(colIndex).toUpperCase();
switch (teradataType) {
...
case TERADATA_TIMESTAMP_WITH_TIME_ZONE: // exact-equals "TIMESTAMP
WITH TIME ZONE"
return LocalTimeType.OFFSET_DATE_TIME_TYPE;
default:
throw CommonError.convertToSeaTunnelTypeError(...);
}
```
See Issue 1 below — this is an exact `switch`/`equals` match with no
tolerance for a precision-qualified type name, unlike the Presto fix two files
away in the same commit.
**`DB2TypeConverter.java`** adds a matching `case` and reconstructs
`sourceType` as `"TIMESTAMP(%d) WITH TIME ZONE"`; correct and consistent with
how the rest of the class already models scale.
**`JdbcFieldTypeUtils.java`** adds a `DBTimestamp`-specific branch (checked
by exact class name, ahead of the generic `Timestamp` branch since `DBTimestamp
extends Timestamp`) that calls `toDBString(true)` via reflection to recover the
offset that `ResultSet.getString()` would otherwise strip for this driver, plus
a `normalizeDb2OffsetDateTimeString` step that truncates DB2's up-to-12-digit
fractional seconds to the 9 digits `java.time` supports — by truncation, not
rounding, so the represented instant is never advanced. This part is
well-reasoned and has a dedicated unit test
(`JdbcFieldTypeUtilsTest.testGetOffsetDateTimeFromDb2TimestampPreservesOffsetAndTruncatesPicos`)
that mocks a real `DBTimestamp` and asserts `getString()` is never called —
good, precise test.
**Key Findings:**
- The normal read path (source query execution → `AbstractJdbcRowConverter`
→ `JdbcFieldTypeUtils.getOffsetDateTime`) is squarely hit by this change; this
is not a defensive/edge-case fix, it's the main path for any table with a
`TIMESTAMP WITH TIME ZONE` column in the four newly-covered dialects.
- Db2 and Presto get precision/format-tolerant matching plus dedicated
value-round-trip tests. Teradata and Vertica get exact-string type-name
matching and zero value-round-trip tests — the two weakest drivers get the
least defensive implementation.
- The fix is precise (not a workaround) for DB2, Presto, and the generic
`GenericTypeConverter`/`JdbcColumnConverter` cases; for Teradata/Vertica it's a
partial fix that reproduces the same "precision embedded in
`getColumnTypeName()`" hazard the PR explicitly solved for Presto two files
over.
- Remaining hole: `JdbcFieldTypeUtils.getOffsetDateTime()`'s pre-existing
generic `Timestamp` fallback (not touched by this PR, but newly reachable for
Teradata/Vertica since before this PR those dialects could never produce a
`TIMESTAMP_TZ`-typed column at all) silently falls back to
`Timestamp.toInstant().atOffset(ZoneOffset.UTC)` — i.e., mislabels a
JVM-default-timezone-shifted instant as UTC — if
`parseOffsetDateTimeFromString` throws on whatever string
`resultSet.getString()` returns for that driver. See Issue 3.
**Runtime path (source-side schema inference → value read, cross-stage
because the inferred `SeaTunnelDataType` flows into every downstream
Transform/Sink):**
```text
Job starts, JDBC source schema discovery
-> CatalogUtils.getTableSchema(metadata, tablePath, dialect)
[CatalogUtils.java:230-238]
-> dialect.getJdbcDialectTypeMapper().mappingColumn(metadata, ...)
-> for Teradata/Vertica/Presto: mappingColumn(BasicTypeDefine) not
overridden
-> throws UnsupportedOperationException
-> catch (UnsupportedOperationException e): columns =
JdbcColumnConverter.convert(metadata, tablePath) [CatalogUtils.java:256-257]
-> isTimestampWithTimeZone(nativeType) reclassifies
effectiveJdbcType
-> TIMESTAMP_WITH_TIMEZONE branch now yields OFFSET_DATE_TIME_TYPE
(was LOCAL_DATE_TIME_TYPE)
(this is table_path= mode; query= mode instead goes through
CatalogUtils.getCatalogTable(ResultSetMetaData, typeMapper, sqlQuery) ->
typeMapper.mapping(metadata, index)
directly, i.e. TeradataTypeMapper/VerticaTypeMapper/PrestoTypeMapper's
own exact-match switch — a
DIFFERENT code path from the tolerant regex above, see Issue 1)
Row read phase
-> AbstractJdbcRowConverter.toInternal(), case TIMESTAMP_TZ
[AbstractJdbcRowConverter.java:121-125]
-> JdbcFieldTypeUtils.getOffsetDateTime(rs, idx)
-> DB2: DBTimestamp branch, toDBString(true) via reflection,
offset preserved
-> everyone else backed by java.sql.Timestamp: re-read as string,
parseOffsetDateTimeFromString
-> on parse failure: silently returns
Timestamp.toInstant().atOffset(UTC) [JdbcFieldTypeUtils.java:210]
```
## 1.2 Compatibility Impact: **Partially incompatible**
- This PR is an incremental extension of an already-documented breaking
change (`docs/en/introduction/concepts/incompatible-changes.md`, "Breaking
Change: Mapping of timezone-aware timestamp columns to `TIMESTAMP_TZ` type",
tracked under `#10685`). That entry already frames the general risk (downstream
sinks expecting `TIMESTAMP` may now get `TIMESTAMP_TZ`).
- What's new and *not* covered by that entry or by this PR's own doc
updates: the `JdbcColumnConverter` fallback change is not scoped to
DB2/Presto/Teradata/Vertica — it changes behavior for **any** dialect that (a)
has no `mappingColumn(BasicTypeDefine)` override and (b) is read in
`table_path=` mode, whenever the driver reports a native type name matching the
new regex or `TIMESTAMPTZ`, or a `DATA_TYPE` code of
`Types.TIMESTAMP_WITH_TIMEZONE`. Before this PR such a column deterministically
mapped to `LOCAL_DATE_TIME_TYPE`; after this PR it maps to
`OFFSET_DATE_TIME_TYPE`. For any already-running job on an unlisted/custom
dialect that happens to match, this silently changes the inferred schema (and,
per the existing ledger entry, can change how Iceberg/Hudi/StarRocks sinks
serialize the column) — with no corresponding bullet added to
`incompatible-changes.md` for this specific fallback path. See Issue 4.
- No config option names, defaults, or SPI method signatures were removed or
renamed. `PrestoDialect.supportsPrimaryKeyMetadata()` overriding the `true`
default to `false` is additive from an API-contract point of view (the
interface method already existed with a default), but changes Presto's own
`table_path=` primary-key-detection behavior — see Issue 6 on scope.
## 1.3 Performance / Side-Effect Analysis
- `JdbcFieldTypeUtils.getOffsetDateTime()` resolves
`obj.getClass().getMethod("toDBString", boolean.class)` via reflection **on
every row**, for every DB2 `TIMESTAMP WITH TIME ZONE` column
(`JdbcFieldTypeUtils.java:189`), instead of caching the `Method` once.
Reflective `getMethod` lookups are not free; for a wide DB2 z/OS extract with
multiple TZ columns and millions of rows this is avoidable per-row overhead.
See Issue 5 (Low, not a correctness bug).
- The new `Pattern` in `JdbcColumnConverter` is a `static final` field,
compiled once — correct.
- No new locks, no new network calls, no unbounded buffers introduced.
## 1.4 Error Handling and Logging
- The DB2 reflection branch correctly wraps `ReflectiveOperationException |
ClassCastException` into a `SQLException` with context (`"Failed to read Db2
timestamp with time zone"`), rather than swallowing it — good.
- `JdbcColumnConverter`'s `default:` branch now reports `effectiveJdbcType`
in the exception message instead of the raw `jdbcType` — a good, deliberate
improvement (the error now reflects the type actually being switched on).
- No sensitive data is logged; no exceptions are swallowed by this PR's own
changes.
- See Issue 3 for the *pre-existing* silent-fallback risk this PR newly
exposes for Teradata/Vertica (not introduced by this PR, but newly reachable
and untested for these two dialects).
**Issue 1: Teradata/Vertica precision-qualified `TIMESTAMP(n) WITH TIME
ZONE` is unrecognized in `query=` mode**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/teradata/TeradataTypeMapper.java:66-101`,
`.../vertica/VerticaTypeMapper.java:100-180`
- **Problem description:** Both mappers do an exact `switch`/`equals`
comparison against `"TIMESTAMP WITH TIME ZONE"` on the upper-cased
`ResultSetMetaData.getColumnTypeName()` value, with no precision-paren
tolerance. `PrestoTypeMapper`, touched in this same commit, explicitly guards
against exactly this hazard (`columnType.startsWith(PRESTO_TIMESTAMP) &&
columnType.contains("WITH TIME ZONE")`, applied before the generic paren-strip)
because Presto reports `timestamp(3) with time zone`. This mapper is used
exclusively by `query =` mode source configs
(`CatalogUtils.getCatalogTable(ResultSetMetaData, typeMapper, sqlQuery)` →
`typeMapper.mappingColumn(metadata, index)` default impl → `mapping(metadata,
colIndex)` directly), which is a different code path from the tolerant
`JdbcColumnConverter` regex fallback used by `table_path=` mode.
- **Potential risk:** If the Teradata/Vertica JDBC driver ever embeds
precision in `getColumnTypeName()` for this type (plausible — this PR's own
`JdbcColumnConverter`-fallback test,
`testTableMetadataFallbackRecognizesProprietaryTypeCodes`, uses `"TIMESTAMP(6)
WITH TIME ZONE"` as the illustrative "teradata" input, and `DB2TypeConverter`
in this same PR constructs source types in the `"TIMESTAMP(%d) WITH TIME ZONE"`
shape), any `query=`-mode job hits `default:` and throws
`CommonError.convertToSeaTunnelTypeError` at schema-discovery time — the column
becomes entirely unreadable via `query=` mode, which is the primary mode
exercised by this PR's own tests (the Presto e2e test uses `query=`, not
`table_path=`).
- **Best improvement:** Apply the same normalization used for Presto
(strip/normalize an optional `(n)` before the switch) to `TeradataTypeMapper`
and `VerticaTypeMapper`, or reuse the `TIMESTAMP_WITH_TIME_ZONE_TYPE_NAME`
regex already added to `JdbcColumnConverter` so all three dialects share one
tolerant matcher instead of three different heuristics.
- **Severity:** Medium (not confirmed with a live Teradata/Vertica driver —
static analysis only, per review scope — but internally inconsistent with the
PR's own fix pattern and its own test fixtures).
**Issue 2: Merge conflict — PR is not currently mergeable against `dev`**
- **Location:** GitHub mergeability state (`mergeStateStatus=DIRTY`,
`mergeable=CONFLICTING`); `compare/dev...9f534f680` reports `status=diverged,
ahead_by=5, behind_by=45`.
- **Problem description:** I ran a live check (not cached): `dev`'s
`seatunnel-e2e/.../JdbcPrestoIT.java` now imports
`org.apache.seatunnel.e2e.common.util.DependencyJar` and calls
`DependencyJar.ofClassName(DRIVER_CLASS)` instead of the `driverUrl()` override
this branch's `JdbcPrestoIT.java` still relies on — confirmed by reading
`dev`'s copy of the file directly. This is an unrelated upstream
E2E-infrastructure refactor that happens to touch the exact file this PR
extends with `testTimestampWithTimeZoneMappingAndValue()`.
- **Potential risk:** Cannot be merged as-is regardless of code correctness;
GitHub will block the merge button and a stale rebase risks silently
reintroducing the old `driverUrl()` pattern dev has since moved away from.
- **Best improvement:** Sync with the latest `dev`, drop the removed
`driverUrl()` override, keep the new
`testTimestampWithTimeZoneMappingAndValue()` method and its imports, adopt
`DependencyJar`-based resolution, then re-run CI to confirm the Presto E2E
cases (including the new TZ test) still pass under the new driver-resolution
mechanism.
- **Severity:** High (blocks merge outright; independent of all code-quality
findings above).
**Issue 3: Shared `Timestamp`-string fallback can silently mislabel a
JVM-shifted instant as UTC, now newly reachable — and untested — for
Teradata/Vertica**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcFieldTypeUtils.java:201-212`
(pre-existing code, not touched by this PR)
- **Problem description:**
```java
if (obj instanceof Timestamp) {
String strVal = resultSet.getString(columnIndex);
if (strVal == null) return null;
try {
return parseOffsetDateTimeFromString(strVal);
} catch (Exception e) {
// Last resort: use the instant-based conversion (may shift by JVM
offset)
return ((Timestamp) obj).toInstant().atOffset(ZoneOffset.UTC);
}
}
```
Before this PR, Teradata and Vertica could never produce a
`TIMESTAMP_TZ`-typed column (both mappers threw
`CommonError.convertToSeaTunnelTypeError` for `TIMESTAMP WITH TIME ZONE`), so
this fallback was never exercised for them. This PR makes it the value-read
path for both dialects, but adds no dedicated test that feeds a realistic
Teradata/Vertica driver `getString()` output through
`parseOffsetDateTimeFromString` to prove it actually succeeds — contrast with
DB2, which got a dedicated `DBTimestamp`-mocking test in this same PR.
I verified the two most likely real-world formats (Vertica Postgres-style
`"...-04"` and Teradata `"...+08:00"`) do parse correctly via the existing
`normalizeOffsetDateTimeString`/direct-ISO paths, so I cannot confirm this
branch is actually hit in practice — but the absence of any test proving it,
for a `catch (Exception e)` block whose failure mode is silent data corruption
rather than a thrown error, is itself the gap.
- **Potential risk:** If a Teradata/Vertica driver ever returns a
`getString()` format this parser doesn't recognize, the job does not fail — it
silently returns a wrong `OffsetDateTime` (JVM-local instant mislabeled as UTC)
with no log line distinguishing this from a genuine UTC value.
- **Best improvement:** Add a `log.warn` (or at minimum a code comment) at
the fallback site noting which dialect/format triggered it, and add a
`JdbcFieldTypeUtilsTest` case per newly-supported dialect (Teradata, Vertica)
that exercises `getOffsetDateTime` with a `Timestamp` + a realistic
driver-formatted string, mirroring the DB2 test already present.
- **Severity:** Medium.
**Issue 4: `incompatible-changes.md` not extended for this PR's
newly-covered dialects**
- **Location:**
`docs/en/introduction/concepts/incompatible-changes.md:10-23` (existing entry,
not touched by this PR)
- **Problem description:** The existing "Breaking Change: Mapping of
timezone-aware timestamp columns to `TIMESTAMP_TZ` type" entry has a dedicated
"Connector-specific behavior changes" bullet list for every dialect a prior PR
touched (Snowflake, StarRocks, Hudi, Xugu, CDC/TiDB, Iceberg). This PR follows
the same underlying breaking-change pattern for DB2, Presto, Teradata, and
Vertica, plus the generic `JdbcColumnConverter` fallback (which is broader than
these four — see 1.2), but adds no bullets to this ledger, and doesn't touch
this file at all.
- **Potential risk:** A user auditing `incompatible-changes.md` before
upgrading (which is the documented, expected workflow — "You need to check this
document before you upgrade to related version") will not learn that their
DB2/Presto/Teradata/Vertica `TIMESTAMP WITH TIME ZONE` columns, or any
custom-dialect column reached via the `JdbcColumnConverter` fallback, now infer
as `TIMESTAMP_TZ` instead of `TIMESTAMP`/failing outright.
- **Best improvement:** Add bullets for DB2, Presto, Teradata, Vertica
following the existing format, and explicitly call out the
`JdbcColumnConverter` generic-fallback behavior change since it's not scoped to
a named dialect.
- **Severity:** Medium.
**Issue 5: DB2 `toDBString` reflection re-resolved every row instead of
cached**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcFieldTypeUtils.java:189`
- **Problem description:** `Method toDBString =
obj.getClass().getMethod("toDBString", boolean.class);` runs inside
`getOffsetDateTime()`, called once per row per `TIMESTAMP_TZ` column via
`AbstractJdbcRowConverter`.
- **Potential risk:** Unnecessary reflective lookup overhead in the
row-conversion hot path for high-volume DB2-for-z/OS batch extracts with TZ
columns. Not a correctness issue.
- **Best improvement:** Resolve the `Method` once (e.g., a
lazily-initialized static field guarded by the class-name check) and reuse it
across rows.
- **Severity:** Low.
**Issue 6: `PrestoDialect.supportsPrimaryKeyMetadata()` is unrelated to this
PR's stated scope**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/presto/PrestoDialect.java:43-46`
- **Problem description:** This flips the default `true` to `false`, meaning
`CatalogUtils.getTableSchema(metadata, tablePath, dialect)` skips
`DatabaseMetaData.getPrimaryKeys()` for Presto in `table_path=` mode. It's a
real, independently-tested fix
(`TimestampWithTimeZoneTypeMappingTest.testPrestoSkipsUnsupportedPrimaryKeyMetadata`)
for a genuine gap — `CatalogUtils.getPrimaryKey` propagates any `SQLException`
from an unsupported `getPrimaryKeys()` call uncaught — but it is not exercised
by the PR's own e2e test (which uses `query=` mode, so this code path is
untouched by any test with a live Presto container) and is unrelated to
`TIMESTAMP_TZ` support.
- **Potential risk:** Low risk of harm (turning the flag off only skips a
call that would otherwise likely throw), but it's scope creep that makes the PR
harder to review/bisect and isn't explained in the PR description.
- **Best improvement:** Either split this into its own PR/commit with its
own rationale, or add a sentence to the PR description explaining why it's
bundled here.
- **Severity:** Low.
**Issue 7: Undocumented removal of `setAutoCommit(false)` in shared e2e test
setup**
- **Location:**
`seatunnel-e2e/.../connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcPrestoIT.java`
(`initializeJdbcConnection`)
- **Problem description:** `this.connection.setAutoCommit(false);` was
removed with no comment. This affects every test method in the class (shared
connection setup), not just the new TZ test, and there's no explanation of why
the explicit transaction mode was dropped.
- **Potential risk:** Low as-is (Presto's `memory` connector likely doesn't
support explicit transactions well, so removing this is plausibly a correctness
fix), but an unexplained behavior change to shared test infrastructure is a
maintainability trap for the next person touching this file.
- **Best improvement:** Add a one-line comment explaining why autocommit is
now left at its default.
- **Severity:** Low.
# 2. Code Quality Assessment
## 2.1 Coding Standards
- All new non-trivial fields/constants have explanatory comments
(`DB2_TIMESTAMP_CLASS_NAME`, `DB2_TIMESTAMP_WITH_TIME_ZONE_PATTERN`,
`TIMESTAMP_WITH_TIME_ZONE_TYPE_NAME`, the Presto/Teradata/Vertica constants) —
good adherence to the "why, not what" standard.
- `isTimestampWithTimeZone` and `normalizeDb2OffsetDateTimeString` are new
non-trivial private methods without their own doc comments (single-line `//`
comments exist at call sites but not on the methods themselves) — minor,
non-blocking per the "boilerplate exempt" carve-out, but these aren't
boilerplate; a short Javadoc on each would meet the bar more cleanly. Not
raised as a numbered issue since the call-site comments already convey intent
adequately.
- Test file `TimestampWithTimeZoneTypeMappingTest.java` uses plain `/* ...
*/` block comments (not Javadoc `/** */`) at class and method level —
acceptable, not the prohibited single-line-`/** */` style.
## 2.2 Test Coverage and Test Stability
- **DB2**: full coverage — type-mapping test
(`Db2TypeConverterTest.testConvertTimestampWithTimeZone`) and value-round-trip
test with a mocked real `DBTimestamp` (`JdbcFieldTypeUtilsTest`) that
specifically asserts `getString()` is never invoked, proving the reflection
path is actually taken. Strong.
- **Presto**: type-mapping unit test + a genuine e2e test against a live
Testcontainers Presto instance
(`JdbcPrestoIT.testTimestampWithTimeZoneMappingAndValue`), asserting both the
mapped type and the parsed value against a known-good literal. Strong.
- **Teradata / Vertica**: only type-mapping unit tests using the bare
(non-precision-qualified) `"TIMESTAMP WITH TIME ZONE"` / `"TIMESTAMPTZ"`
strings — no e2e test (no Teradata/Vertica Testcontainers image is realistic to
run in this repo's CI, understandably), and no `JdbcFieldTypeUtilsTest`
value-round-trip test at all. Combined with Issue 1 and Issue 3, this means the
two weakest-covered dialects are also the two with the least tolerant
implementation.
- No `Thread.sleep`, no unclosed resources, no order-dependent assertions,
no floating-point-without-delta, and no non-deterministic assertions found in
any of the added/modified tests. The e2e readiness signal
(`given().ignoreExceptions().await()...untilAsserted(...)` polling a `CREATE
TABLE IF NOT EXISTS _readiness_probe`) is pre-existing and unchanged.
- **Rating: Risk present** — not because any added test is flaky, but
because a real Teradata/Vertica regression in Issue 1 or Issue 3 would not be
caught by this PR's own CI suite (path: `TeradataTypeMapper.java:95-96`,
`VerticaTypeMapper.java:161-163`, no corresponding e2e or value-level unit
test).
## 2.3 Documentation Updates
- `docs/en/connectors/source/Jdbc.md` and the `zh` equivalent gained a clear
"Timestamp with time zone" section listing exactly which native types map to
`TIMESTAMP_TZ`, matching the code (Presto `timestamp(p) with time zone` —
matches the precision-tolerant Presto fix; Teradata is listed without a `(p)`
qualifier, consistent with `TeradataTypeMapper`'s exact-match implementation,
so the doc is at least internally honest about the current code's limitation
even though it doesn't call it out as a limitation).
- `docs/en/connectors/sink/Jdbc.md` / `zh` correctly disclose that DB2
auto-table-creation isn't supported for this type, consistent with
`DB2TypeConverter.reconvert()` having no `OFFSET_DATE_TIME_TYPE` case.
- Not updated: `incompatible-changes.md` — see Issue 4.
# 3. Architectural Soundness
## 3.1 Elegance
Mixed: DB2 and Presto are **Precise fixes** (root-caused to the actual
driver quirk, with matching tests). Teradata/Vertica are **partial fixes** —
they add the SeaTunnel-side type mapping but don't close the same
driver-format-variance gap the PR closes for Presto in the identical commit, so
they land closer to a **temporary workaround** for the common case than a fully
robust fix.
## 3.2 Maintainability
Reasonable — each dialect's type name constants and matching logic stay
localized to its own `TypeMapper`/`TypeConverter` class, consistent with the
existing per-dialect architecture (no leakage of dialect-specific logic into
the engine or shared core, in line with this repo's connector guidelines). The
one exception is `JdbcColumnConverter`'s new `isTimestampWithTimeZone` regex,
which duplicates (with a superset pattern) the same "precision may be embedded"
concept that `PrestoTypeMapper` now also implements independently and
`TeradataTypeMapper`/`VerticaTypeMapper` don't implement at all — three
different answers to the same problem in one PR is a maintainability smell
worth converging (see Issue 1's suggested fix).
## 3.3 Extensibility
Good — the pattern (add a `TIMESTAMP_WITH_TIME_ZONE` constant + switch case
per dialect) is easy for the next contributor to replicate for any remaining
dialect not yet covered.
## 3.4 Historical-Version Compatibility
Already-running jobs that read a `TIMESTAMP WITH TIME ZONE`/`TIMESTAMPTZ`
column through DB2, Presto (bare form), Teradata (bare form), or the
`GenericTypeConverter` path were previously either broken (threw
`UnsupportedOperationException`/`CommonError`) or, for the
`JdbcColumnConverter` fallback specifically, silently returning `LocalDateTime`
— see 1.2 for the full compatibility classification and Issue 4 for the
documentation gap this leaves.
# 4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 2 | PR is not mergeable against `dev` (real content conflict in
`JdbcPrestoIT.java`, caused by unrelated upstream E2E-infra refactor) | GitHub
mergeability state / `JdbcPrestoIT.java` | High |
| 1 | Teradata/Vertica `query=`-mode type matching doesn't tolerate
precision-qualified `TIMESTAMP(n) WITH TIME ZONE`, unlike the Presto fix in the
same PR | `TeradataTypeMapper.java:66-101`, `VerticaTypeMapper.java:100-180` |
Medium |
| 3 | Pre-existing silent UTC-mislabeling fallback newly reachable, and
untested, for Teradata/Vertica | `JdbcFieldTypeUtils.java:201-212` | Medium |
| 4 | `incompatible-changes.md` not extended for this PR's dialects / the
generic fallback change |
`docs/en/introduction/concepts/incompatible-changes.md` | Medium |
| 5 | DB2 reflection `Method` lookup re-done every row instead of cached |
`JdbcFieldTypeUtils.java:189` | Low |
| 6 | `PrestoDialect.supportsPrimaryKeyMetadata()` is scope creep relative
to the PR's stated purpose | `PrestoDialect.java:43-46` | Low |
| 7 | Undocumented removal of `setAutoCommit(false)` in shared e2e test
setup | `JdbcPrestoIT.java` (`initializeJdbcConnection`) | Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers** (must be fixed):
- Issue 2 (High) — sync with `dev` and resolve the `JdbcPrestoIT.java`
conflict; this alone prevents merging today regardless of everything else.
- Issue 1 (Medium) — either confirm (with real Teradata/Vertica driver
testing, which I could not do here per this review's static-analysis-only
scope) that the exact-match type name is actually correct for both drivers, or
add the same precision-tolerant normalization Presto got in this PR.
- Issue 3 (Medium) — at minimum, add value-round-trip tests for
Teradata/Vertica proving the shared string-parsing fallback handles their
actual `getString()` output; ideally also add a log line at the silent-fallback
site.
2. **Recommended fixes** (non-blocking):
- Issue 4 — extend `incompatible-changes.md` to match the existing
per-dialect bullet pattern.
- Issue 5 — cache the DB2 reflection `Method` lookup.
- Issue 6 — explain or split out the
`PrestoDialect.supportsPrimaryKeyMetadata()` change.
- Issue 7 — comment the `setAutoCommit` removal.
**Overall assessment**: The core idea and the
DB2/Presto/GenericTypeConverter implementations are solid, precisely scoped,
and well-tested — the DB2 reflection-based offset recovery in particular is a
careful piece of work with a test that proves the right code path is taken. The
gap is consistency: the PR fixes a specific "precision embedded in the type
name" driver quirk for Presto but leaves the identical class of quirk
unaddressed for Teradata and Vertica, and doesn't add the value-level test
coverage for those two dialects that would either prove it's a non-issue or
catch the regression. Given this is genuinely difficult to fully validate
without live Teradata/Vertica instances, **Option A** (land this PR as-is after
fixing the merge conflict, and file a fast follow-up specifically for
Teradata/Vertica precision-tolerance + value-round-trip tests) is a reasonable
alternative to **Option B** (hold the whole PR until Teradata/Vertica reach
parity with DB2/Presto) — but Opti
on B is what I'd request before merge, since the inconsistency is demonstrable
from the PR's own diff and test fixtures without needing a live driver, and the
fix (reusing the existing `TIMESTAMP_WITH_TIME_ZONE_TYPE_NAME` regex or the
Presto normalization pattern) is small.
---
This is a technical self-review of my own PR (author = DanielLeens), posted
as a plain issue comment rather than a GitHub Review because GitHub rejects
review submissions on one's own PR (422). It does not satisfy the repository's
human-approval gate; a write-capable maintainer still needs to complete that
step. CI is currently green (`Build`, `Notify test workflow`, `labeler` all
`pass` as of this check), so the only hard blocker right now is the `dev` merge
conflict (Issue 2).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
