DanielLeens commented on PR #11485:
URL: https://github.com/apache/seatunnel/pull/11485#issuecomment-5340759230
Tenth-round self-check, performed against the current head
`9f534f6800ad6f1a476452e56511bb167726734b`. I confirmed this is byte-for-byte
the same head my previous (ninth) round reviewed on 2026-08-16 — same 17 files,
same `+315/-4` diff stat against the merge base, no new commit and no new
discussion activity since then. Per this repository's rule that a given code
version gets exactly one complete review, I am not re-deriving or re-publishing
the full issue list against an unchanged version; that review still stands as
the current technical conclusion for this diff, and both of its blockers remain
open and unaddressed:
- **Issue 1 (High)** — Teradata/Vertica `TIMESTAMP_TZ` values flow
exclusively through a fallback that silently fabricates a `+00:00` offset on
parse failure, with zero value-path coverage (and zero *possible* coverage for
Teradata in this repo's CI).
- **Issue 2 (Medium)** — the Teradata/Vertica `query =` discovery path
cannot match `TIMESTAMP(n) WITH TIME ZONE`, which this PR's own test fixture
treats as a realistic driver response.
What I did re-verify live this round, rather than trust the prior snapshot:
**CI.** Unchanged and still fully green. `Build`, `Notify test workflow` and
`labeler` are all `completed/success` on this exact SHA, with `Build` showing
the same completion timestamp (`2026-08-13T21:02:21Z`) as last round. CI is not
a blocker.
**Merge state — this is new information not covered in any of the previous
nine rounds.** GitHub currently reports `mergeable_state=dirty` (a real content
conflict), and a `compare` against `dev` shows `status=diverged`, `ahead_by=5`,
`behind_by=41`. I ran a local dry-run merge against the latest `apache/dev` to
find the exact cause rather than guessing: the only real conflict is in
`JdbcPrestoIT.java` — the same file this PR extends with
`testTimestampWithTimeZoneMappingAndValue()`. The root cause is upstream PR
#11572 ("[Improve][E2E] Resolve container dependencies through Maven"), which
landed on `dev` after this branch's last sync and refactored every JDBC E2E
class's driver-jar resolution from an overridden `driverUrl()` method to a
`DependencyJar.ofClassName(...)` call — including `JdbcPrestoIT.java`. So this
is unrelated upstream E2E-infrastructure drift touching the same file by
coincidence, not a semantic disagreement with this PR's own `TIMESTAMP_TZ`
logic; the res
olution is mechanical (drop the removed `driverUrl()` override, keep the new
test method and its imports, adopt the new `DependencyJar` import ordering).
**Updated status: three blockers now, not two.**
1. Issue 1 (High) — unresolved, carried over from round nine.
2. Issue 2 (Medium) — unresolved, carried over from round nine.
3. **New: merge conflicts with `dev` (`mergeable_state=dirty`).** Please
sync with the latest `dev` and resolve the conflict in `JdbcPrestoIT.java`
first. Once synced, rerun CI to confirm the `DependencyJar`-based driver
resolution still passes the Presto E2E cases this PR extends.
As before, this is a technical self-check on my own PR, recorded as a plain
comment because GitHub does not permit self-approval. It does not satisfy the
repository approval gate. Conclusion is unchanged from round nine: **not ready
to merge** until Issue 1 and Issue 2 are resolved, now with the additional
prerequisite of syncing `dev` and resolving the `JdbcPrestoIT.java` conflict.
--
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]
