SEZ9 commented on PR #11971: URL: https://github.com/apache/seatunnel/pull/11971#issuecomment-6307988357
Summarizing the earlier review points so they're easy to track: **High** - **F1 – `YashanDbCatalog.isSinglePhysicalTableQuery` closes the shared cached connection.** The connection from `getConnection(defaultUrl)` is cached by the catalog, so closing it poisons the cache and the new merge step can't succeed for YashanDB. Please avoid closing the cached connection (or use a dedicated connection for this probe). - **F2 – OceanBase/YashanDB overrides run the full user query just to read metadata.** Please bound the query (e.g. a row limit) so large tables can't cause long stalls or OOM during planning. **Medium** - **F3 – verified origin `TablePath` is discarded.** The merge should use the `TablePath` actually verified from metadata rather than whatever path the query-derived `TableIdentifier` carries; otherwise PK/comments can be merged from the wrong table. - **F4 – schema-only drivers produce a null database name.** `tableExists`/`getTable` throws in that case and the merge silently never applies. Please handle the null-database case explicitly. - **F7 – query prepared/executed twice on the catalog path.** The no-catalog path already reuses a single `ResultSetMetaData`; please align the catalog path with it. - **F8 – fallbacks swallow exceptions at DEBUG.** Since the merge is an advertised feature, please log fallbacks at WARN with the reason so users can diagnose why metadata wasn't preserved. **Docs / compatibility** - **F5 / F6 – changelog and `incompatible-changes.md`.** Query-only pipelines gain a merged primary key on upgrade, which changes split planning and sink auto-create DDL. Please record this in the incompatible-changes notes alongside the `Jdbc.md` changes. Happy to take another pass once these are addressed. Thanks! <!-- streview-comment:1922 --> -- 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]
