DanielLeens commented on PR #11485:
URL: https://github.com/apache/seatunnel/pull/11485#issuecomment-5379364883
Fresh-run check-in, independently re-verified against the bare mirror rather
than assumed. As the author, GitHub does not permit self-approval, so this is
recorded as a plain issue comment and does not satisfy the repository approval
gate.
**What changed since my last review (2026-08-21T14:23:37Z):** nothing on the
code side. `headRefOid` is still `9f534f6800ad6f1a476452e56511bb167726734b`, no
new commits, no new comments, no other reviewer has weighed in (there are zero
inline review threads and every review on this PR is mine). I did not take that
on faith — I diffed `dev...refs/pr-review/11485` directly against the bare
mirror (17 files, 315 insertions / 4 deletions across `docs/`, `connector-jdbc`
main+test sources, and `connector-jdbc-e2e-part-7`) and re-read every changed
file's current content from the object store, not from memory of the prior
round. My conclusion is unchanged — this is still **not ready to merge** — but
I re-derived every finding against the actual current source below, and I'm
also flagging one thing my last review didn't check: the PR's live mergeability.
**New in this round: the PR is not mergeable.** `gh pr view` currently shows
`mergeStateStatus: DIRTY`, `mergeable: CONFLICTING`, `isDraft: true`. The
branch is `61` commits behind `dev` (`ahead_by=5, behind_by=61`, `diverged`).
None of my prior reviews checked this — I'd only verified CI, which is green
but is a fork-side pointer against a stale base. This needs a rebase onto
current `dev` before it can even be test-merged, independent of the two open
code issues below.
# What Problem Does This PR Solve?
- **User pain point:** before this PR, `TIMESTAMP_TZ`-typed JDBC columns
either failed discovery outright (Teradata, Vertica, Db2 for z/OS — no mapping
case, `CommonError.convertToSeaTunnelTypeError`), produced a column with an
unhandled JDBC 4.2 `Types.TIMESTAMP_WITH_TIMEZONE` code, or were silently
flattened to timezone-naive `TIMESTAMP` with the offset discarded and no error
(Presto's precision-qualified type name, and the shared legacy metadata
fallback used by every dialect without its own `mappingColumn(BasicTypeDefine)`
implementation).
- **Fix approach:** add the missing type-mapping cases in five places
(generic JDBC 4.2 converter, Db2, Presto, Teradata, Vertica); canonicalize
Presto's precision-qualified type name before the pre-existing paren-truncation
step; recognize proprietary native type names (`TIMESTAMPTZ`, `TIMESTAMP(n)
WITH TIME ZONE`) in the shared legacy fallback via a regex; add a Db2-specific
value-read branch that recovers the offset the IBM driver's `getString()`
normally strips.
- **One-sentence summary:** the Db2, Presto, and generic-JDBC-4.2 legs of
this PR are correctly and rigorously implemented and tested end-to-end, but
Teradata and Vertica are given the exact same "offset preserved" contract in
the docs and type mappers while their actual *value*-read path is untested and
can silently fall back to a conversion the code's own comment says is
unreliable — that gap is why I still can't approve this.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Re-read from the current head, not assumed:
- **Generic JDBC 4.2** (`GenericTypeConverter.java:170-173`):
`Types.TIMESTAMP_WITH_TIMEZONE` now maps to `OFFSET_DATE_TIME_TYPE`.
`reconvert()` in this class throws `UnsupportedOperationException`
unconditionally for every type (pre-existing, unrelated to this PR) — no
asymmetry introduced here.
- **Db2 for z/OS** (`DB2TypeConverter.java:224-229`):
`DB2_TIMESTAMP_WITH_TIME_ZONE = "TIMESTAMP WITH TIME ZONE"` is a new switch
case in `convert()` mapping to `OFFSET_DATE_TIME_TYPE`. I checked whether the
matching `reconvert()` (sink schema-creation direction) has a `TIMESTAMP_TZ`
case — it does not, so a `TIMESTAMP_TZ` column written to a Db2 sink with
auto-create-table enabled would hit `default:` and throw
`CommonError.convertToConnectorTypeError`. This looked like a real gap until I
checked the docs: `docs/en/connectors/sink/Jdbc.md` explicitly states *"Auto
table creation remains available only for dialects that provide a catalog and a
timezone-aware type converter; Db2 does not currently provide JDBC auto table
creation."* So this is a disclosed, deliberate scope boundary, not an
overlooked bug — good that it's documented rather than silently discovered by a
user.
- **Value-read path for Db2** (`JdbcFieldTypeUtils.java:186-196`):
`com.ibm.db2.jcc.DBTimestamp` is detected by class name and its offset is
recovered via reflection (`toDBString(true)`) rather than a direct import, even
though `db2jcc` is already a `compile`-scope dependency of this module
(confirmed in `connector-jdbc/pom.xml`, unchanged by this PR) and the new unit
test (`JdbcFieldTypeUtilsTest.java`) imports `DBTimestamp` directly. The
reflection isn't wrong — it fails loud via `SQLException` if the method
signature ever changes — but it's more defensive than necessary given the
direct dependency is already there and already used in the test. Low-severity,
non-blocking style note.
- **Presto** (`PrestoTypeMapper.java:69-75`):
`columnType.startsWith(PRESTO_TIMESTAMP) && columnType.contains("WITH TIME
ZONE")` canonicalizes `timestamp(3) with time zone` to the exact-match constant
*before* the pre-existing `"(" `-index truncation runs. I checked the ordering
is load-bearing: without it, `"TIMESTAMP(3) WITH TIME ZONE".split("\\(")[0]`
would produce `"TIMESTAMP"`, colliding with plain `TIMESTAMP` and silently
dropping the offset — exactly the bug this PR fixes. Verified end-to-end via a
real Presto container in
`JdbcPrestoIT.testTimestampWithTimeZoneMappingAndValue`, which pins
`timeZoneId=UTC` on the JDBC URL and asserts an exact `OffsetDateTime` (not
just "didn't throw").
- **Teradata / Vertica** (`TeradataTypeMapper.java:95-96`,
`VerticaTypeMapper.java:161-163`): metadata mapping is correct for the
*exact-string* case (`"TIMESTAMP WITH TIME ZONE"` / `"TIMESTAMPTZ"`), but see
Issues 1 and 2 below — both the value-read path and part of the discovery path
have real gaps that Db2 and Presto don't.
- **Legacy metadata fallback** (`JdbcColumnConverter.java`):
`TIMESTAMP_WITH_TIMEZONE` is correctly split out of the previously-shared
`TIMESTAMP` case (was: both mapped to `LOCAL_DATE_TIME_TYPE`, silently dropping
the offset for any dialect using this fallback and reporting JDBC type `-101`).
I also checked the new `isTimestampWithTimeZone(nativeType)` regex
(`TIMESTAMP(?:\s*\(\s*\d+\s*\))?\s+WITH\s+TIME\s+ZONE`, used with `.matches()`
against the fully-uppercased, trimmed native type name) against Oracle's
`TIMESTAMP(n) WITH LOCAL TIME ZONE`, which is a *different* semantic type
(session-relative, no explicit per-row offset) — the regex requires `WITH`
immediately followed by `TIME`, so `"...WITH LOCAL TIME ZONE"` does not match
and is correctly left alone. Oracle has its own dedicated `OracleTypeConverter`
anyway, so this fallback wouldn't apply to it in practice, but it's good that
the regex doesn't create a latent trap if a similar LTZ-style dialect string
ever reaches this
path.
**Full runtime path for the still-open value-path gap** (Issue 1), traced
from `AbstractJdbcRowConverter` down to the exact fallback branch, re-verified
line-for-line against the current head:
```text
AbstractJdbcRowConverter.toInternal(...)
-> case TIMESTAMP_TZ:
[AbstractJdbcRowConverter.java:121-125]
-> JdbcFieldTypeUtils.getOffsetDateTime(rs, idx)
[JdbcFieldTypeUtils.java:163-]
-> resultSet.getObject(columnIndex)
-> not OffsetDateTime / ZonedDateTime / Instant / DBTimestamp
-> instanceof Timestamp branch
[JdbcFieldTypeUtils.java:198-212]
-> resultSet.getString(columnIndex) //
Teradata/Vertica drivers land here
-> parseOffsetDateTimeFromString(strVal)
-- driver renders a numeric offset ("+08:00") -> parses
correctly
-- driver renders a region-ID zone (e.g. "...
Asia/Shanghai") -> parse throws
-> catch (Exception e):
return ((Timestamp)
obj).toInstant().atOffset(ZoneOffset.UTC)
-- this is exactly the conversion the method's own comment
two lines above
warns against: "Avoid using Timestamp.toInstant()
directly because the
Timestamp was constructed with JVM-default-timezone
semantics, which would
shift the value by the JVM offset" — used here as a
silent last resort,
with no log, no exception, no error code
```
**Key findings:**
1. This is not a boundary/exception-only path for Teradata/Vertica — it is
their *only* value-read path for `TIMESTAMP_TZ` columns, reached on every
normal row read once discovery has (correctly) mapped the column to
`OFFSET_DATE_TIME_TYPE`. Before this PR, such columns never reached this code
at all (discovery failed loudly first, per
`CommonError.convertToSeaTunnelTypeError`); this PR's own metadata fix is what
routes them into a value-read path that was hardened for Db2 (reflection-based,
driver-specific) and Presto (proven via real container) but not for them.
2. `Timestamp.toInstant()` on a `java.sql.Timestamp` that a driver
constructed via JVM-default-timezone semantics from a TZ-aware SQL value can
represent the **wrong instant**, not merely the right instant with a mislabeled
offset — that's precisely why the surrounding code has a comment warning not to
use it directly, yet the catch-block fallback does exactly that, silently.
3. Teradata's own IT test is `@Disabled("Disabled because it needs user's
personal teradata account to run this test!")` (`JdbcTeradataIT.java:37`) and
Vertica's is `@Disabled` pending a usable container image
(`JdbcVerticaIT.java:38`) — confirmed by reading both files directly. So the
two dialects with the weakest value-path guarantee are also the two with no
possible way to catch a regression here in this repo's CI, now or in the future
without infrastructure changes.
4. Separately: `TeradataTypeMapper`/`VerticaTypeMapper`'s
`mapping(ResultSetMetaData, int)` switches on the raw, un-canonicalized
`getColumnTypeName(...)` with no paren-stripping — so it cannot match a
precision-qualified name like `TIMESTAMP(6) WITH TIME ZONE`. I confirmed this
PR's own
`TimestampWithTimeZoneTypeMappingTest.testTableMetadataFallbackRecognizesProprietaryTypeCodes`
asserts Teradata reports exactly `"TIMESTAMP(6) WITH TIME ZONE"` — but that
assertion goes through `JdbcColumnConverter`'s regex-based fallback, not
through `TeradataTypeMapper.mapping()` itself, where the same
precision-qualified string would fall to `default:` and throw.
`JdbcColumnConverter` (regex) and `PrestoTypeMapper` (explicit
canonicalization) both handle this correctly; Teradata/Vertica's own mappers do
not.
## 1.2 Compatibility Impact
**Partially incompatible in effect, not in API/config/SPI/serialization.**
No option, public API, or checkpoint/savepoint state is touched. The one real
behavioral flip: `JdbcColumnConverter.java` splits `TIMESTAMP_WITH_TIMEZONE`
out of the previously-shared `TIMESTAMP` case in the generic fallback, so any
existing job whose driver reports raw JDBC type `-101` through that fallback
path — not just the five dialects named in this PR's title, but *any*
mapper-less dialect routed there — will see its inferred schema flip from
`TIMESTAMP` to `TIMESTAMP_TZ` on the next catalog discovery.
`docs/en/introduction/concepts/incompatible-changes.md` already has a general
"Breaking Change: Mapping of timezone-aware timestamp columns to `TIMESTAMP_TZ`
type" entry from an earlier PR (#10685, same class of change, same author) with
per-dialect sub-bullets (Snowflake, StarRocks, Hudi, CDC, Iceberg, Xugu) — this
PR does not add a sub-bullet there for the newly-covered dialects (Db2, Presto,
T
eradata, Vertica, and the generic-fallback case), even though it follows the
exact same established pattern that file's own convention calls for. This is a
real, checkable documentation-completeness gap against the project's own
established practice for this exact change class (Issue 5).
## 1.3 Performance / Side-Effect Analysis
No new I/O, threads, or locks on a hot path; the regex/string
canonicalization in `JdbcColumnConverter`/`PrestoTypeMapper` runs once per
column at discovery time, not per row.
`PrestoDialect.supportsPrimaryKeyMetadata()` now returning `false` changes
runtime behavior for every Presto `table_path`-based job by skipping
`getPrimaryKeys(...)` entirely in `CatalogUtils.getTableSchema(...)` — I traced
this is a real, connected fix (not scope creep): `getPrimaryKey(...)` is
exactly what previously ran unconditionally in that method, and this PR's own
new test `testPrestoSkipsUnsupportedPrimaryKeyMetadata` confirms the change is
deliberate. It's undisclosed in the PR description/docs, though — worth a
one-line explanation of *why* Presto's PK metadata query needed to be skipped
(Issue 7, non-blocking).
## 1.4 Error Handling and Logging
See Issue 1 above/below — the silent fallback is the substantive
error-handling gap in this diff. Everything else in the diff either throws
loudly (`CommonError.convertToConnectorTypeError`/`convertToSeaTunnelTypeError`
for unmapped types, `SQLException` for the Db2 reflective-call failure path) or
is a pure, side-effect-free mapping addition.
**Issue 1: Teradata/Vertica `TIMESTAMP_TZ` values flow exclusively through a
fallback that can silently produce the wrong instant, with zero achievable
value-path test coverage**
- Location: `TeradataTypeMapper.java:95-96`,
`VerticaTypeMapper.java:161-163` (routing), `JdbcFieldTypeUtils.java:201-212`
(the actual fallback)
- Problem: neither driver returns a `java.time` object or a
`DBTimestamp`-like wrapper for a TZ column, so both land in the generic
`Timestamp` branch, which re-reads via `getString()` and, on any unparseable
rendering (e.g. a region-ID zone name instead of a numeric offset), silently
substitutes `Timestamp.toInstant().atOffset(UTC)` — the exact conversion the
method's own adjacent comment warns can shift the value by the JVM's default
offset.
- Potential risk: for a feature whose entire purpose is offset preservation,
a previously loud discovery-time failure (`CommonError`) becomes a silent,
plausible-looking, potentially wrong value at row-read time, specifically on
the two dialects with no possible CI coverage in this repo today (Teradata
`@Disabled`, needs a personal account; Vertica `@Disabled`, no usable container
image).
- Best improvement: Option A — implement real, driver-verified
`TIMESTAMP_TZ` read overrides for Teradata/Vertica (mirroring the Db2 approach)
backed by driver-mocked unit tests that exercise the actual value path, not
just metadata mapping. Option B — scope Teradata and Vertica out of this PR's
mappers and docs entirely, and land Db2/Presto/generic-JDBC-4.2 now (all three
are proven with real evidence); revisit Teradata/Vertica once their value path
has real coverage. I'd lean toward B given neither dialect can be exercised in
this repo's CI as things stand.
- Severity: High
- Raised by another reviewer: No — my own finding.
**Issue 2: Teradata/Vertica metadata mapping can't match a
precision-qualified type name, contradicting this PR's own test fixture**
- Location: `TeradataTypeMapper.java:66,95`,
`VerticaTypeMapper.java:100,161-162`
- Problem: both mappers switch on the raw, un-canonicalized column type
name, so only the exact string `TIMESTAMP WITH TIME ZONE` matches, while
`JdbcColumnConverter`'s regex and `PrestoTypeMapper`'s explicit
canonicalization both correctly accept the precision-qualified form
`TIMESTAMP(n) WITH TIME ZONE` — which this PR's own
`TimestampWithTimeZoneTypeMappingTest` fixture asserts Teradata actually
reports (just through a different code path than the one that would really run
for a `query =`-configured Teradata source).
- Potential risk: a Teradata/Vertica source configured with `query =` (a
mainstream configuration path, not the catalog/table_path path) throws
`CommonError.convertToSeaTunnelTypeError` on exactly the columns this PR
advertises as newly supported. Loud, not silent, hence Medium rather than High.
- Best improvement: extract a shared canonicalization helper (generalizing
`JdbcColumnConverter`'s regex or `PrestoTypeMapper`'s prefix-check) and call it
from both mappers' `mapping(...)` overrides, with a test that drives it through
the mappers themselves rather than only through the fallback.
- Severity: Medium
- Raised by another reviewer: No.
# 2. Code Quality Assessment
## 2.1 Coding Standards
License headers, AOSP/Spotless formatting, no wildcard imports, no
`System.out.println`, no special-symbol comments — all compliant on inspection.
Comments explain *why* rather than restating code (e.g. the ordering-constraint
comment in `PrestoTypeMapper`, the JVM-offset-shift warning in
`JdbcFieldTypeUtils`). Non-blocking:
`PrestoDialect.supportsPrimaryKeyMetadata()` has no explanatory comment for
*why* Presto needs this override (1.3); the new Presto/Teradata/Vertica
canonicalization calls plain `toUpperCase()` rather than
`toUpperCase(Locale.ROOT)`, inconsistent with `JdbcColumnConverter`'s own
`Locale.ROOT` usage added in this same PR (Issue 6, Low).
## 2.2 Test Coverage and Test Stability
**Stability rating: Stable.** The Db2 unit test
(`JdbcFieldTypeUtilsTest.testGetOffsetDateTimeFromDb2TimestampPreservesOffsetAndTruncatesPicos`)
and the Presto E2E (`JdbcPrestoIT.testTimestampWithTimeZoneMappingAndValue`)
are both deterministic: no `Thread.sleep`, `LIMIT 1` is safe against a fixed
3-row fixture, the Presto assertion pins the session zone via `timeZoneId=UTC`
on the JDBC URL rather than depending on the runner's default zone, and both
use strict `OffsetDateTime`/`assertEquals` comparisons rather than a weak
"didn't throw" check. No flaky-test anti-patterns in the new/changed test code.
Coverage gap — this is Issue 1's other half: Teradata has zero achievable
value-path coverage in this repo today; Vertica's IT is disabled pending a
container image. The new `TimestampWithTimeZoneTypeMappingTest` for both
dialects only exercises `mapping(ResultSetMetaData, int)` via mocks
(metadata/schema-shape correctness), never the row-value read that Issue 1
concerns — so the new tests genuinely improve discovery-time confidence but say
nothing about the specific gap that's blocking this PR.
## 2.3 Documentation Updates
`docs/en`/`docs/zh` `Jdbc.md` (source and sink) are updated in parallel and
structurally consistent between languages. I spot-checked the factual claims
against the code: the Db2 auto-table-creation limitation and the
12-fractional-digit truncation are both accurate. Gap: the docs give an
unqualified "value is represented by `java.time.OffsetDateTime`" guarantee
across all five dialects with no caveat for the two (Teradata, Vertica) whose
value path is unproven per Issue 1 — and, per 1.2, no incompatible-changes.md
sub-bullet for this PR's specific dialects (Issue 5, Medium, non-blocking on
its own but connected to Issue 1's severity).
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Each change lands at the layer that owns the concern — per-dialect
mappers/converters for schema mapping, the shared
`AbstractJdbcRowConverter`/`JdbcFieldTypeUtils` for value conversion, the
generic fallback for mapper-less dialects. The Presto reordering fix is a
small, surgical correction to a genuine pre-existing bug. The one structural
inconsistency: type-name recognition for the "with time zone" pattern is
expressed three different ways across this PR (regex in the shared fallback,
explicit prefix-check in Presto, exact-string switch in Teradata/Vertica) —
this PR synced Presto's canonicalization but not Teradata/Vertica's, which is
the direct root cause of Issue 2. A single shared predicate used by all four
native-type-name-based mappers would remove this class of drift permanently —
this is a **precise fix for Db2/Presto/generic**, but a **partial/incomplete
fix for Teradata/Vertica** given Issues 1 and 2.
## 3.2 Maintainability
Solid overall: constants carry rationale, ordering constraints are commented
where they matter. `normalizeDb2OffsetDateTimeString` is invoked from the
shared `parseOffsetDateTimeFromString` rather than scoped to the Db2-only call
site — harmless given how specific its regex is (it only matches Db2's
`yyyy-mm-dd-hh.mm.ss` external format), but it's slightly wider coupling than
strictly needed. The reflective `toDBString(boolean)` call has no compile-time
safety net against a driver upgrade changing that method's signature, but fails
loud via `SQLException` rather than silently, which is the right failure mode
if it ever breaks.
## 3.3 Extensibility
The per-dialect mapper/converter split and the new
`supportsPrimaryKeyMetadata()` hook are both clean extension seams. Extracting
the shared type-name-recognition predicate discussed in 3.1 would directly
resolve Issue 2 and make the next dialect's "with time zone" support trivial to
add correctly.
## 3.4 Historical-Version Compatibility
No serialization, checkpoint/savepoint, config option, or public API surface
is touched by this diff — a job restored from an existing checkpoint is
unaffected at the state level. The only historical-compatibility consideration
is the inferred-schema flip covered in 1.2, which the project's existing
general `incompatible-changes.md` entry already covers as a change class; a
per-dialect sub-bullet for this PR's five dialects would be a precision
improvement consistent with that file's own established convention, not a
net-new compatibility requirement.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|-------|----------|----------|
| 1 | Teradata/Vertica `TIMESTAMP_TZ` values flow only through a fallback
that can silently produce the wrong instant on parse failure; zero achievable
value-path test coverage | `TeradataTypeMapper.java:95-96`,
`VerticaTypeMapper.java:161-163`, `JdbcFieldTypeUtils.java:201-212` | High |
| 2 | Teradata/Vertica metadata mapping can't match `TIMESTAMP(n) WITH TIME
ZONE`, contradicting this PR's own test fixture |
`TeradataTypeMapper.java:66,95`, `VerticaTypeMapper.java:100,161-162` | Medium |
| 3 | No incompatible-changes.md sub-bullet for this PR's specific dialects,
despite the project's own established convention for this exact change class |
`docs/en/introduction/concepts/incompatible-changes.md` | Medium |
| 4 | `PrestoDialect.supportsPrimaryKeyMetadata()` behavior change is real
and tested but undisclosed/uncommented as to *why* | `PrestoDialect.java:43-46`
| Low |
| 5 | Db2 `DBTimestamp` access uses reflection despite `db2jcc` already
being a compile-scope dependency used directly in this PR's own test |
`JdbcFieldTypeUtils.java:186-196` | Low |
| 6 | Presto/Teradata/Vertica canonicalization uses locale-sensitive
`toUpperCase()` instead of `Locale.ROOT`, inconsistent with
`JdbcColumnConverter` in the same PR | `PrestoTypeMapper.java:71`,
`TeradataTypeMapper.java:66`, `VerticaTypeMapper.java:100` | Low |
| 7 | Docs give an unqualified `OffsetDateTime`-preservation guarantee for
all five dialects with no caveat for the two whose value path is unproven |
`docs/en(zh)/connectors/source/Jdbc.md` | Low, follows from Issue 1 |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
1. **Blockers — must be fixed**
- Issue 1 (High): give Teradata/Vertica a real, driver-verified
`TIMESTAMP_TZ` value-read path, or scope both out of this PR's mappers/docs and
land the proven Db2/Presto/generic-JDBC-4.2 work now.
- Issue 2 (Medium, blocking as-is because it leaves the same two dialects
half-delivered against their own advertised contract): make Teradata/Vertica's
metadata mapping recognize the precision-qualified form. This dissolves
automatically if Issue 1 is resolved via the scope-out option.
- **Gate, not code:** the branch must be rebased onto current `dev` —
it's currently `61` commits behind with `mergeable: CONFLICTING` /
`mergeStateStatus: DIRTY`. This is independent of Issues 1-2 and needs to
happen regardless of which fix direction is chosen.
2. **Recommended fixes — non-blocking**
- Issues 3-7 as itemized above (incompatible-changes.md sub-bullet,
`PrestoDialect` disclosure comment, Db2 reflection-vs-direct-import
simplification, `Locale.ROOT` consistency, doc caveat scoped to the proven
dialects).
**Option A vs Option B:** (A) finish Teradata/Vertica's value-path properly
in this PR — the row-converter seam already exists, so this is plausible; or
(B) land the proven Db2/Presto/generic-JDBC-4.2 work now and split
Teradata/Vertica into a follow-up once their value path has real, achievable
coverage. I'd lean toward (B): it ships three of five dialects against fully
verified evidence (a real Presto container with exact-instant assertions, and
Db2's reflection-based offset recovery which I traced end-to-end against the
actual `DBTimestamp` class this module already depends on) without letting two
currently-untestable dialects hold up work that's already solid.
**CI:** green on the current head (`Build`, `Notify test workflow`,
`labeler` all pass) — not itself a blocker, and unaffected by any of the issues
above since none of them are exercised by CI-reachable tests.
**Overall assessment:** the Db2, Presto, and generic-JDBC-4.2 legs of this
PR are genuinely well-built and thoroughly verified — the Presto ordering fix
in particular closes a real silent-data-loss bug with solid container-based
proof, and the Db2 doc already gets ahead of its own auto-table-creation
limitation rather than leaving users to discover it. But Teradata and Vertica
are given the identical "offset preserved" promise in the docs and type mappers
as the three proven dialects, without the value-path correctness or achievable
test coverage to back that promise — and that's a real risk for a feature whose
entire point is not silently losing timezone information. I'd merge confidently
today if this were scoped to Db2/Presto/generic, or if Issue 1 were closed with
real coverage for Teradata/Vertica; as-is, I'm not approving my own PR.
As the author, this review is a technical self-check recorded as a plain
comment; it does not count toward the approval gate. A write-capable maintainer
still needs to review and approve once Issues 1-2 and the rebase are resolved.
--
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]
