SEZ9 commented on PR #11971: URL: https://github.com/apache/seatunnel/pull/11971#issuecomment-5611938710
@DanielLeens thanks for tracing the history on this re-review round — that is exactly the information I needed. Your diff of `bbb76bd99a` against the pre-merge head, restricted to `connector-jdbc`, `Jdbc.md` and `incompatible-changes.md`, showing zero changes to this PR's own files, tells me the new head is a pure conflict-resolution merge. I agree it does not warrant a fresh code re-review on its own, and I also saw your follow-up that the `Build` check on `bbb76bd99a` has completed successfully with no new commits since. One consequence of that, though: because the merge did not touch any PR files, the state of the findings from my earlier review is exactly what it was at `6bafd798fc`. Nothing in this thread tells me which of them were addressed before that head, so before I give the write-side approval I need a short status per item: - **PR11971-F1 (HIGH)** – `YashanDbCatalog.isSinglePhysicalTableQuery` closing the shared cached connection from `getConnection(defaultUrl)`, which poisons the catalog connection cache and makes the merge step always fail for YashanDB. - **PR11971-F2 (HIGH)** – the OceanBase/YashanDB overrides executing the full user query with no row limit just to read metadata. - **PR11971-F3 / F4 (MEDIUM)** – in `JdbcCatalogUtils`, the metadata-verified origin `TablePath` being discarded in favour of whatever the query-derived `TableIdentifier` carries, and the null database name for schema-only drivers causing `tableExists`/`getTable` to throw so the merge silently never applies. - **PR11971-F5 / F6 (MEDIUM)** – the changelog and `incompatible-changes.md` entries for the automatic metadata merge and the newly merged primary key on upgrade. You mention `incompatible-changes.md` is among the PR's files, so if it now covers this, a pointer is enough. - **PR11971-F7 / F8 (MEDIUM)** – the catalog path preparing/executing the query twice, and all fallback paths swallowing exceptions at DEBUG level. If these were resolved in commits leading up to `6bafd798fc`, a quick "fixed in …" per item (or "not fixed, here is why") is all I need; I will then re-check only those spots rather than re-auditing the PR. If any are still open, F1 and F2 are the ones I consider blocking, the rest can be argued either way. Thanks again for the careful work on the re-review scope — it made this round much easier. <!-- streview-comment:940 --> -- 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]
