yzeng1618 commented on PR #11971: URL: https://github.com/apache/seatunnel/pull/11971#issuecomment-6409456517
Thank you @nzw921rx for the real-database test matrix and @DanielLeens for re-verifying everything against the current head — both reviews are now addressed in `a89a3e35cf`. Point-by-point: **P1: `ALL` one-sided-join cardinality.** Agreed that the origin check cannot see the FROM/JOIN shape, and rejecting all such queries would also reject true single-table queries (indistinguishable in `ResultSetMetaData`). Chosen boundary: `ALL` is now explicitly a *user-asserted* mode — a WARN is logged at merge time (documenting the one-sided-join hazard and its upsert-collapsing consequence), the option/enum Javadoc, en/zh `Jdbc.md` and `incompatible-changes.md` all state it, and `testAllModeMergesOneSidedJoinOriginPerUserAssertion` pins the behavior as deliberate so any future change to the semantics is a visible diff. `COMMENT`/`NONE` do not merge keys and stay safe for such queries. **P1: Driver support boundary.** With your (and Daniel's) endorsement of the smaller scope, the feature now documents its per-driver support matrix instead of claiming generic support: MySQL family supported (verified on real databases); PostgreSQL (table-only origin, no catalog/schema), Oracle/SQL Server/DuckDB (blank origin table name) documented as "use `table_path` + `query`" with a note that SQL Server/DuckDB catalogs do not expose the table comment. The matrix is in en/zh `Jdbc.md` and summarized in `incompatible-changes.md`; the option description and `getPhysicalTablePath` contract Javadoc state the same boundary. **P1: SAP HANA mapping.** `SapHanaCatalog` now overrides `resolveQueryTable` to return the query-derived table with an empty verified origin path, with Javadoc explaining the databaseName-as-schema mapping mismatch — the merge is explicitly disabled for HANA until the namespace mapping is aligned (and it remains real-test-required before re-enabling), so a query on `TEST.T` can no longer load `SYSTEM.T`. **Issue 9: helper contract.** `CatalogUtils.getPhysicalTablePath` now carries a contract Javadoc: null on missing table identity, blank catalog/schema normalized to null, and dialect default/mapping is the caller's responsibility. **P2: branch state.** The branch was synced with `dev` on 2026-10-25 (conflict resolved in the OceanBase import section only); GitHub now reports the PR as MERGEABLE. Your review was against `4d4a6ecf`, which predates the sync and the two subsequent fix commits (`4a08829d5` default `NONE` + docs; `a89a3e35cf` this round). Left as documented future work (not silently dropped): dialect-aware origin resolution for PostgreSQL/Oracle/SQL Server/DuckDB with real-driver-family ITs, HANA namespace alignment, and re-visiting whether `ALL` can gain a structural join guard. CI for `a89a3e35cf` is running; I'll keep the branch green. -- 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]
