DanielLeens commented on PR #11485:
URL: https://github.com/apache/seatunnel/pull/11485#issuecomment-5350342350
Eleventh-round self-check, against the current head
`9f534f6800ad6f1a476452e56511bb167726734b` — still byte-identical to the
version my last full review (2026-08-16) and the tenth-round self-check
covered: same 17 files, same `+315/-4` diff stat against the merge base, no new
commit and no new discussion since then. Re-deriving and re-posting the same
full issue list against an unchanged version wouldn't add anything, so this is
a live re-verification of the two open blockers plus current CI/merge health
rather than a full rewrite:
- **Issue 1 (High, still open)** —
`JdbcFieldTypeUtils.getOffsetDateTime()`'s `Timestamp` branch
(`seatunnel-connectors-v2/connector-jdbc/.../utils/JdbcFieldTypeUtils.java`) is
the only value-read path Teradata and Vertica `TIMESTAMP_TZ` columns go through
(neither driver returns `OffsetDateTime`/`ZonedDateTime`/`Instant`, or a
recognizable proprietary wrapper class like Db2's `DBTimestamp`, so both fall
into the generic `Timestamp` branch). On a string-parse failure that branch
silently falls back to `((Timestamp) obj).toInstant().atOffset(ZoneOffset.UTC)`
— the comment right above it even says this "may shift by JVM offset" — yet the
result is stamped with a hard-coded `+00:00` offset regardless of the JVM's
actual zone or the column's real stored offset. For a connector whose entire
purpose is "don't lose the timezone," fabricating an offset on the failure path
is a correctness regression with no logging and no test covering it (Teradata
isn't in this repo's CI at all, an
d the Vertica type-mapping test only checks metadata mapping, not the parsed
value).
- **Issue 2 (Medium, still open)** — `TeradataTypeMapper.mapping()` and
`VerticaTypeMapper.mapping()` `switch` on the raw, unstripped
`getColumnTypeName()` string (`TERADATA_TIMESTAMP_WITH_TIME_ZONE = "TIMESTAMP
WITH TIME ZONE"`, `VERTICA_TIMESTAMP_WITH_TIME_ZONE` likewise), with no
precision-stripping like `PrestoTypeMapper` added in this same PR
(`columnType.startsWith(...) && columnType.contains("WITH TIME ZONE")`). If
either driver reports the realistic `TIMESTAMP(n) WITH TIME ZONE` form — which
is exactly the form `JdbcColumnConverter`'s new regex fallback in this same PR
was written to handle — the switch falls to `default`, which throws
`CommonError.convertToSeaTunnelTypeError(...)`, a `SeaTunnelRuntimeException`.
That's not an `UnsupportedOperationException`, so
`CatalogUtils.getTableSchema()`'s `catch (UnsupportedOperationException e)`
fallback to `JdbcColumnConverter.convert()` never triggers — the job fails hard
on the `query =` discovery path instead of silentl
y degrading.
What I re-verified live this round rather than trust the prior snapshot:
**CI** — unchanged, still fully green (`Build`, `Notify test workflow`,
`labeler` all pass on this SHA).
**Merge state** — still `mergeable_state=dirty` / `CONFLICTING`. The
divergence from `dev` has grown since the last check: `ahead_by=5`,
`behind_by=45` (was `behind_by=41` a few days ago). This branch needs a rebase
before it can be merged regardless of the two issues above, and the gap is only
getting wider the longer it sits.
### Conclusion: Not recommended for merge
Both previously-identified blockers are still unaddressed in this exact
diff, and the branch is now further behind `dev` than before. Recommended next
step is the same as prior rounds: fix Issue 1 by not fabricating an offset on
parse failure (fail loudly instead, matching how the DB2 path throws on a
reflection failure), fix Issue 2 by stripping precision before the
Teradata/Vertica `switch` the same way `PrestoTypeMapper` now does, add
value-path test coverage for at least Vertica, then rebase onto current `dev`
before requesting merge.
--
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]
