DanielLeens commented on PR #11971: URL: https://github.com/apache/seatunnel/pull/11971#issuecomment-6359805371
Thanks @nzw921rx for running this against real databases and for the detailed matrix. This is exactly the evidence the mocked `ResultSetMetaData` unit tests cannot give us. I did not run the databases myself (my check is static, nothing was executed locally); I re-read the code on head `4d4a6ecfca20` to see whether your findings line up with it, and they do. **P1 PostgreSQL / Oracle / SQL Server / DuckDB no-op.** Consistent with the code. `CatalogUtils.getSinglePhysicalTablePath` / `getPhysicalTablePath` (connector-jdbc `CatalogUtils.java:345-382`) returns empty as soon as `ResultSetMetaData.getTableName` is blank, which matches your Oracle / SQL Server / DuckDB result. For PostgreSQL, where the table name is reported but catalog and schema are blank, `JdbcCatalogUtils.mergeWithUnderlyingTable` returns `tableOfQuery` early when both database and schema are blank (the `isBlank(getDatabaseName) && isBlank(getSchemaName)` guard). So for these drivers the `COMMENT` default silently does nothing, and the docs (`Jdbc.md:50`) currently describe the behavior generically without saying which database families actually resolve a table. **P1 `ALL` with a one-sided join.** Also consistent. The only guard is "every result column reports the same origin table", so `SELECT t.* FROM t JOIN t2 ...` passes it, and `ALL` would then merge `t`'s primary key and options even though the join can multiply rows. `Jdbc.md:50` says the merge is skipped for "detected multi-table queries", but nothing detects this shape. `COMMENT` stays safe, which supports keeping it as the default. **P1 SAP HANA and MySQL-only real-DB coverage.** I agree these need an owner decision; the HANA schema-vs-database mapping in particular should not ship untested. Suggested next steps for @yzeng1618 (the branch needs a rebase anyway, see below): 1. Decide the scope. Either make the PostgreSQL case work (fill the default schema/database from the catalog before the blank-both early return, plus a `JdbcPostgresIT` query-only case), or document explicitly that the feature currently resolves MySQL-family drivers only. 2. For Oracle / SQL Server / DuckDB, either require `table_path` and say so in the docs, or resolve the table another way; do not rely on `ResultSetMetaData.getTableName`. 3. For `ALL`, add a join guard (or at least a documented warning plus a join test). Keep `COMMENT` as the default. 4. Add a per-database support matrix for `query_table_metadata_merge` to the docs, and either a HANA-specific override or a HANA-specific test. 5. Rebase on current `dev`: GitHub reports `mergeable_state: dirty` (head ahead 12, behind 101 versus `dev`), so the conflicts need resolving and CI needs to run on the new head. I have not pushed any verdict on top of your change request. Once a new head is pushed I will do a full fresh pass over the whole diff against the points above. -- 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]
