DanielLeens commented on PR #11485:
URL: https://github.com/apache/seatunnel/pull/11485#issuecomment-5287979075
Seventh-round self-review note: the head advanced from `6c4babb8d` (my
2026-08-13 11:05 review) to `9f534f6800` (current), and per this repo's policy
that a genuine new commit requires a fresh full re-review rather than a
rubber-stamp, I re-derived the diff from scratch instead of assuming the new
commit was a no-op.
What I verified:
1. **The new commit is a pure merge-sync with zero content change to this
PR's diff.** `9f534f6800` is `Merge branch 'dev' into
davidzollo_timestamp-tz-connectors-20260718`, bringing in roughly a dozen
unrelated `dev` commits (doc improvements for other connectors, a Jackson
shading fix, workflow changes). I diffed each of the 17 files this PR touches
at both the old head (`dev...6c4babb8d`) and the new head (`dev...9f534f6800`)
and compared the two diffs with git blob hashes and hunk line numbers stripped
out: the content is byte-for-byte identical (`diff` reports no differences
after normalizing `index` and `@@` lines). The only visible changes were
hunk-header line-number shifts caused by unrelated upstream doc edits elsewhere
in `docs/en/connectors/source/Jdbc.md` and `docs/zh/connectors/source/Jdbc.md`.
`git diff apache/dev...9f534f6800 --stat` still reports the same 17 files, 315
insertions / 4 deletions as my last round.
2. **Every source-level claim from my 2026-08-13 review still holds against
the current head**, re-checked directly rather than trusted:
`TeradataJdbcRowConverter.java` and `VerticaJdbcRowConverter.java` are both
still bare 20-line passthrough classes overriding only `converterName()` —
neither has a `TIMESTAMP_TZ` read/write override.
`seatunnel-e2e/.../connector-jdbc-e2e-part-2/.../JdbcTeradataIT.java:37` is
still `@Disabled("...needs user's personal teradata account...")`. So Issue 1
(High) — the Teradata/Vertica `TIMESTAMP_TZ` read path has no dialect override
and its only fallback (`JdbcFieldTypeUtils.getOffsetDateTime`'s generic
`Timestamp` branch) silently fabricates a UTC offset via `((Timestamp)
obj).toInstant().atOffset(ZoneOffset.UTC)` on any string-parse miss, with zero
test coverage for Teradata and zero *possible* CI coverage until that IT is
re-enabled — is unchanged and still the sole blocker. Issues 2-5 (Medium/Low,
sink-side conformance risk, doc overstatem
ent, missing `PrestoDialect` comment, uncached reflection) are likewise
unchanged; nothing in the diff moved.
3. **No new community input to incorporate.** `gh api
repos/apache/seatunnel/pulls/11485/reviews`, `.../comments`, and
`repos/apache/seatunnel/issues/11485/comments` all still return only my own 6
prior self-reviews and zero comments from any other user.
CI diagnosis (this round's real change): the apache-side "Build" check on
the current head (`9f534f6800`) again shows `failure`. I traced the pointer to
the actual fork run rather than trusting the badge:
`repos/apache/seatunnel/check-runs/94444856847` points at
`DanielLeens/seatunnel` run `31699053139` (attempt 1, completed). That run has
exactly one failing job: `updated-modules-integration-test-part-1 (11,
ubuntu-latest)`. The Maven failure is `connector-cdc-postgres-e2e` →
`PostgresCDCIT.testPostgresCdcSnapshotOnlyAndCommittedOffsetStartupModes` →
`ConditionTimeout` at line 629, an `await().atMost(120, TimeUnit.SECONDS)`
waiting on a replication-slot restore after `savepointJob`/`restoreJob`. The
test's own inline comment (`PostgresCDCIT.java:627-628`, on `dev`, pre-existing
and untouched by this PR) says: "Restoring the checkpoint and reconnecting the
existing replication slot can take longer on shared GitHub runners than the
initial CDC startup" — i.e. this is a docume
nted, known-flaky timing class of failure on shared runners, not a new
regression. This module is pulled into the reactor because Postgres-CDC's sink
side is JDBC-backed (`postgrescdc_to_postgres_with_debezium_to_kafka.conf`
writes through the Postgres JDBC sink), so `connector-cdc-postgres-e2e`
legitimately depends on `connector-jdbc` and gets included by
`update_modules_check.py`. But none of the columns exercised in that test
(`f_big`, plain CDC row fields) are `TIMESTAMP_TZ`-typed, and this PR only
touches the `TIMESTAMP_TZ`-specific branches of `GenericTypeConverter`,
`JdbcColumnConverter`, `DB2TypeConverter`, `PrestoTypeMapper`/`PrestoDialect`,
`TeradataTypeMapper`, `VerticaTypeMapper`, and
`JdbcFieldTypeUtils.getOffsetDateTime` — none of which are on the code path a
checkpoint/restore/replication-slot test exercises. I confirmed no other job in
the fork run failed (single failing job, single attempt). Conclusion: the
current CI red is a real failure, but it is unrelated to
this PR's diff and does not add a new blocker beyond Issue 1; it should be
retried at the job level rather than treated as evidence against this PR.
Conclusion: Ready to merge after fixes still stands, unchanged from
2026-08-13. The one Blocker (Issue 1, High — implement real
`TeradataJdbcRowConverter`/`VerticaJdbcRowConverter` `TIMESTAMP_TZ` read
overrides with driver-mocked test coverage, or scope Teradata/Vertica out of
this PR's mapper and docs until that lands) remains open and unaddressed in the
current head. As the author, this self-review records a technical conclusion
only and does not count toward the approval gate; a write-capable maintainer
still needs to review, and Issue 1 should be resolved before merge regardless
of CI outcome on the unrelated Postgres-CDC timing failure.
--
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]
