DanielLeens commented on PR #11971: URL: https://github.com/apache/seatunnel/pull/11971#issuecomment-5617127216
@SEZ9 Status per item, verified against the source at `9e057517bf54` (the commit that closed all eight findings) and re-confirmed unchanged through the current head `bbb76bd99a`, which, as you noted, touches none of this PR's own files: - **F1** (YashanDB closing the shared cached connection) — fixed. `YashanDbCatalog.resolveQueryTable` keeps `getConnection(defaultUrl)` outside the try-with-resources, with only the `Statement` inside it, plus a Javadoc note that the connection is cached/shared and must not be closed there. Confirmed present at the current head. - **F2** (unbounded second query execution) — fixed. The old two-call shape (`getTable` plus a separate `isSinglePhysicalTableQuery`) was replaced with a single `resolveQueryTable` that OceanBase and YashanDB each override once, so the query now executes exactly once, capped with `setMaxRows(1)`. One residual, already-disclosed caveat I'm carrying as a non-blocking Medium follow-up, not fixed by this PR: `setMaxRows(1)` bounds the client-side result but may not stop a MySQL-protocol driver from buffering the full server-side result before truncating. - **F3/F4** (verified origin TablePath discarded / null database name) — fixed. `mergeWithUnderlyingTable` now takes the verified `Optional<TablePath>` from `resolveQueryTable` directly instead of re-deriving one from the query's `TableIdentifier`, and `JdbcCatalogUtils.withDefaultDatabase` fills in a missing database name from the catalog default before the `tableExists`/`getTable` lookup. - **F5/F6** (incompatible-changes.md) — fixed. Both `docs/en` and `docs/zh` `incompatible-changes.md` gained a JDBC Connector entry covering the merge behavior, the split-planning implication, and the workaround. - **F7/F8** (double execution / debug-level swallowing) — fixed. Same root cause as F2 (now one execution), and every fallback catch in `resolveQueryTable`/`getSinglePhysicalTablePath`/`mergeWithUnderlyingTable` logs at warn with the table path and exception instead of debug. All eight were re-traced against the raw diff at the time, not taken on the author's word: see my 2026-08-31 review for the itemized verification against `9e057517bf54`, and my 2026-09-09 re-review for the re-confirmation on a later head. Since this PR's files haven't changed since, the status above still holds on `bbb76bd99a`. From my side there's no open blocker. The one thing I'd still like closed as a fast-follow, not a merge blocker, is confirming whether `setMaxRows(1)` actually limits server-side transfer for the OceanBase driver. I have read-only rights here, so once you're satisfied a write-capable maintainer can take it from here. -- 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]
