[yzeng1618]

Purpose of this pull request

Fix #11969 (same root cause as the historical report #8329).

When a JDBC source table is defined by query only, the schema is derived from JDBC ResultSetMetaData, which carries no column/table comments, constraint keys or partition keys. Sinks that auto-create tables (Doris, StarRocks, MySQL, ...) therefore create tables without any comments.

This PR adds a new source option query_table_metadata_merge (NONE / COMMENT / ALL, default NONE) for query-only sources. When enabled and the query maps to a single existing table (verified from the query result metadata), the missing metadata is merged into the query-derived table by reusing the existing mergeCatalogTable logic — the same behavior as configuring table_path together with query.

Key properties, shaped by the review discussion:

• NONE is the default: unconfigured jobs behave exactly as before this option existed, with zero extra metadata probing. COMMENT merges only column/table comments (descriptive only); ALL additionally merges table options, primary key, constraint keys and partition keys (the full table_path + query merge, opt-in).
• Single-table verification is origin-based: every selected column must report the same origin table. Joins that select columns from multiple tables are skipped; computed/expression columns leave the whole query unverifiable (merge skipped). The check cannot see the FROM/JOIN shape, so a one-sided join (SELECT t.* FROM t JOIN t2 ...) is not detected — ALL merges its keys based on the user's assertion that the query preserves the underlying table's row cardinality (warned at merge time and documented); COMMENT/NONE stay safe for such queries.
• Driver support boundary: the verification requires the driver to report the origin table of every result column. Verified working for MySQL-family drivers. PostgreSQL reports a table name but no catalog/schema, Oracle/SQL Server/DuckDB report no origin table name, and SAP HANA's catalog maps namespaces differently (the merge is explicitly disabled for it) — for these databases use table_path + query, which already worked. The support matrix is documented in the Jdbc source docs (en/zh).
• The verified origin TablePath (not the query-derived identifier) is loaded; a blank database name is filled from the catalog default before tableExists/getTable; any verification or load failure falls back to the query-derived table with a WARN log.

Does this PR introduce any user-facing change?

Only opt-in. New source option query_table_metadata_merge (NONE default / COMMENT / ALL). Unconfigured query-only jobs are byte-for-byte unchanged. Jobs that opt in get auto-created sink tables carrying the underlying table's comments (COMMENT) or comments/options/keys (ALL). The behavior is documented in the Jdbc source docs (en/zh) and in incompatible-changes.md (en/zh).

How was this patch tested?

• Unit tests in JdbcCatalogUtilsTest / CatalogUtilsTest / JdbcSourceTableConfigTest: single-table resolution, join rejection, expression-column unverifiable, metadata-failure fallback, verified-origin-path loading, withDefaultDatabase cases, table_path + query ignoring the mode, ALL one-sided-join boundary (pinned as user-asserted), mode defaults and per-table override.
• JdbcMysqlTableOptionsIT: real-database E2E for the MySQL family — query-only source with COMMENT merge; asserts comments are present in SHOW CREATE TABLE of the auto-created sink table.

Check list

• If any new Jar binary package adding in your PR, please add License Notice according
  New License Guide
• If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
• If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.

[DanielLeens]

What Problem Does This PR Solve?

When a JDBC source table is defined purely with query (no table_path), the schema was previously derived only from ResultSetMetaData, which carries no column/table comments, constraint keys, or partition keys. Downstream sinks that auto-create tables (Doris, StarRocks, MySQL, ...) therefore lost comments and keys that existed on the real source table. This PR resolves the physical table the query maps to (when identifiable) and reuses the existing mergeCatalogTable logic to backfill that missing metadata into the query-derived schema, mirroring what already happens when a user configures table_path together with query.

1. Code Change Review

1.1 Core Logic Analysis

JdbcCatalogUtils.getCatalogTable(...) (catalog path, line ~275, and no-catalog fallback, line ~477) now calls a new helper:

static CatalogTable mergeWithUnderlyingTable(
        CatalogTable tableOfQuery, UnderlyingTableLoader underlyingTableLoader) {
    TableIdentifier tableId = tableOfQuery.getTableId();
    if (tableId == null || StringUtils.isBlank(tableId.getTableName())) return tableOfQuery;
    TablePath tablePath = tableId.toTablePath();
    if (TablePath.DEFAULT.equals(tablePath) || (blank(db) && blank(schema))) return tableOfQuery;
    try {
        CatalogTable tableOfPath = underlyingTableLoader.load(tablePath);
        if (tableOfPath == null || tableOfPath.getTableSchema().getColumns().isEmpty()) return tableOfQuery;
        return mergeCatalogTable(tableOfPath, tableOfQuery);   // pre-existing merge logic, unchanged
    } catch (Exception e) {
        return tableOfQuery;  // defensive fallback, logged at debug
    }
}

tableOfQuery.getTableId() comes from CatalogUtils.getCatalogTable(ResultSetMetaData, ...) (pre-existing, CatalogUtils.java:349-351), which derives table/database/schema only from column index 1 (metadata.getTableName(1)), with no verification that the other selected columns originate from the same physical table.

Key Finding: this "does the query map to a single physical table" detection is a first-column heuristic, not a real single-table check. For a join whose first selected column belongs to a real, existing table (a very common shape, e.g. SELECT o.id, o.amount, c.name FROM orders o JOIN customers c ...), tableExists(tablePath) succeeds and mergeCatalogTable will attempt to merge orders' primary key / constraint keys / comment into the joined result — even though the joined rows are not uniquely keyed by orders.id (a one-to-many join can repeat that value). mergeCatalogTable's guard (columnKeysOfQuery.containsAll(primaryKeyOfPath.getColumnNames())) only checks that a column of the same name exists in the query result — it does not verify the value actually originates from, or is unique like, the source table's PK column.

Consequence: for the no-catalog fallback path (getCatalogTable(Connection, JdbcSourceTableConfig, JdbcDialect), line ~477), this is a new behavior — previously query-only, no-catalog sources never attached any PK/constraint metadata at all (CatalogUtils.getCatalogTable(ResultSetMetaData, typeMapper, sqlQuery) has never looked up a DatabaseMetaData primary key). After this PR, an existing production job using a join in query-only mode, without a JDBC catalog configured, can silently start reporting a primary key it doesn't actually satisfy. If a sink (e.g. MySQL/Doris/StarRocks auto-create) turns that into a PRIMARY/UNIQUE KEY on the newly-created target table, subsequent inserts of legitimately-duplicated join rows will start failing with duplicate-key errors that never happened pre-upgrade.

For the catalog-enabled path this specific PK risk is not new — MySqlCatalog.getTable(String) already calls CatalogUtils.getCatalogTable(Connection, String, JdbcDialectTypeMapper), which already infers a PK the same first-column way (this is exactly what the existing doc section "Query and primary-key caution" already warns about). What is new even there is that comments, constraint keys, and partition keys are now also merged in via the same unverified table resolution — extending the blast radius of an already-known-risky heuristic.

Runtime path (discovery time only, not the hot data path): getTables() → getCatalogTable(tableConfig, jdbcCatalog|connection, ...) → mergeWithUnderlyingTable → underlyingTableLoader.load (an extra tableExists + getTable round trip) → mergeCatalogTable. This only runs once during source table discovery/planning, so the added JDBC round trips are not a hot-path performance concern.

1.2 Compatibility Impact

Partially incompatible. Row shape/column order is preserved (the schemaEquals branch in mergeCatalogTable iterates the query's column order, so SeaTunnelRow structure and checkpoint/savepoint restore compatibility are not affected). However, the produced CatalogTable's primary key / constraint keys / comment can change for any existing query-only source whose query happens to resolve (via column 1) to a real table — most notably for join queries, and most notably for previously-catalog-less deployments where no such inference existed before at all. This is a silent, non-opt-outable behavior change for a widely-used feature (query-only JDBC source), and can produce data-correctness regressions (spurious unique/PK constraints on newly auto-created sink tables) for already-deployed join-based pipelines. Already-created sink tables (from before the upgrade) are unaffected since table DDL only runs once.

1.3 Performance / Side-Effect Analysis

Negligible. The extra tableExists/getTable calls happen only once per configured table during source planning, not per record or per checkpoint. The broad catch (Exception e) with debug-level logging is a deliberate, reasonable defensive fallback and does not swallow Errors.

1.4 Error Handling and Logging

Issue 1: Location JdbcCatalogUtils.java:493-522 (mergeWithUnderlyingTable) and its trigger sites at lines ~275-281 and ~477-481. Problem: "does the query map to a single physical table" is decided solely from ResultSetMetaData.getTableName(1) (column 1) plus a table-existence check, with no verification that all selected columns actually originate from that one table. Potential risk: for join queries (a mainstream, documented use of query-only mode) whose first column belongs to a real table, PK/constraint-key/comment metadata from that one table gets silently attached to a multi-table result set; if a sink auto-creates a target table with that key, legitimately duplicated join output rows can fail with duplicate-key errors — a correctness/availability regression for existing production jobs, worst for the no-catalog fallback path where no such metadata was ever attached before. Best improvement: verify that every selected column's table/schema/catalog (via ResultSetMetaData.getTableName(i) for all i, where the driver reports it) matches column 1 before treating the query as single-table, or add a config switch to opt in/out of this metadata merge for query-only sources; at minimum, extend the existing "Query and primary-key caution" documentation to explicitly cover the new comment/constraint-key/partition-key merge, and add a regression test with a join query-only source proving the merge is safely skipped or correctly scoped. Severity: High. Raised by another reviewer: No.

Issue 2: Location JdbcCatalogUtils.java:524-527 (UnderlyingTableLoader interface). Problem: new package-private functional interface has no Javadoc explaining why a custom @FunctionalInterface was introduced instead of java.util.function.Function (answer: it needs to declare throws Exception). Potential risk: low, purely a readability/maintenance nit. Best improvement: one-line Javadoc. Severity: Low. Raised by another reviewer: No.

2. Code Quality Assessment

2.1 Coding Standards

The new mergeWithUnderlyingTable method has a clear, well-written Javadoc explaining purpose and fallback behavior. The UnderlyingTableLoader functional interface lacks a comment (Issue 2 above). Naming and structure are consistent with the surrounding class; reuse of the existing mergeCatalogTable avoids duplicating the merge logic, which is a good minimal-footprint choice.

2.2 Test Coverage and Test Stability

New unit tests in JdbcCatalogUtilsTest (6 cases) thoroughly cover mergeWithUnderlyingTable's own boundary conditions: resolvable single-table query, unresolvable table name, missing database/schema, table not existing, underlying table with an empty schema, and loader-throws fallback. All are deterministic pure-function tests with no sleeps or shared state. The extended JdbcMysqlTableOptionsIT adds deterministic string-content assertions against SHOW CREATE TABLE output, no timing dependency. Test Stability rating: Stable. Test blind spot: no test (unit or E2E) exercises a join/multi-table query in query-only mode to confirm the merge is safely scoped or skipped — this is exactly the scenario in Issue 1 and should be covered before merge.

2.3 Documentation Updates

docs/en and docs/zh for the Jdbc source are both updated with an accurate description of the new merge behavior. Recommend adding a cross-reference to the existing "Query and primary-key caution" section so users relying on join queries in query-only mode are aware the same caution now also applies to comments/constraint/partition-key merging.

3. Architectural Soundness

3.1 Elegance of the Solution

The reuse of the existing mergeCatalogTable function to unify "explicit table_path+query" and "implicit table resolved from query" is an elegant, minimal-footprint design that avoids new merge logic. The weak point is inherited, not introduced: the "single table" detection it now automatically triggers on was never designed to be a reliable single-table check.

3.2 Maintainability

Good — small, self-contained helper with a functional-interface seam that is unit-testable without a real JDBC catalog/connection, as demonstrated by the new tests.

3.3 Extensibility

Fine; the UnderlyingTableLoader abstraction cleanly supports both the catalog-backed and connection-only code paths without duplicating logic.

3.4 Historical-Version Compatibility

Row/column structure and column order are preserved, so checkpoint/savepoint restore for already-running jobs is not affected by this change. The risk is confined to newly-derived CatalogTable metadata (PK/constraint/comment) used at source-table-discovery and sink-auto-create time, as analyzed in 1.2.

4. Issue Summary

Number	Issue	Location	Severity
1	"Single physical table" detection relies only on column-1 metadata; can misattribute a join's first table's PK/constraint keys onto a multi-table result, silently changing auto-create sink DDL and risking duplicate-key failures for existing join-based query-only jobs	JdbcCatalogUtils.java:493-522, CatalogUtils.java:349-351	High
2	UnderlyingTableLoader functional interface has no Javadoc	JdbcCatalogUtils.java:524-527	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed

   • Issue 1 (High): add a check that every selected column resolves to the same table (or at least document the extension of the existing join-safety caution and add a join-query regression test) before treating a query as mapping to "a single physical table," to avoid silently attaching wrong-scope PK/constraint metadata for join-based query-only sources.

2. Recommended fixes — non-blocking

   • Issue 2 (Low): add a short Javadoc to UnderlyingTableLoader explaining why it isn't java.util.function.Function.
   • Cross-link the doc addition to the existing "Query and primary-key caution" section.

Overall assessment: the fix directly and elegantly solves the reported comment/metadata-loss problem for the common SELECT * FROM single_table case, reusing existing, already-tested merge logic, and is well covered by new unit tests for its own boundary conditions. The one real gap is that the "single table" precondition it relies on was never actually validated to be single-table, which — for join queries, a mainstream use of query-only sources — can silently and non-opt-outably introduce incorrect key metadata that previously did not exist (most notably for no-catalog fallback deployments). This is worth closing before merge given it affects a widely used, already-in-production code path; the rest of the PR is solid, well-tested, and well-documented.

[yzeng1618]

Addressed the review findings in commit 281fe76:

• Validate every result column's catalog/schema/table before merging underlying-table metadata. Detected joins, inconsistent origins, and missing origin metadata now keep the query-derived table unchanged.
• Apply the guard to both catalog-backed and direct JDBC fallback paths, including the Statement-based metadata paths used by OceanBase and YashanDB.
• Add the requested UnderlyingTableLoader Javadoc and link the EN/ZH documentation to the existing query primary-key caution.
• Add regression coverage for same-table metadata, a multi-table JOIN result, and skipping the metadata loader for multi-table queries.

Validation:

• .\mvnw.cmd -o spotless:apply
• .\mvnw.cmd -o -pl seatunnel-connectors-v2/connector-jdbc -Dskip.spotless=true -Dcheckstyle.skip=true -Dtest=CatalogUtilsTest,JdbcCatalogUtilsTest test (32 tests, 0 failures, 0 errors)

[DanielLeens]

Thanks for the ping and for the quick turnaround, @yzeng1618! I re-reviewed the latest head (281fe76d1) against my previous round on fe3c866fd, retraced the full call chain again from scratch, and checked the fix against Issue 1 from my earlier review.

What Problem Does This PR Solve?

• User pain point: when a JDBC source table is defined purely with query (no table_path), the schema was previously derived only from ResultSetMetaData, which carries no column/table comments, constraint keys, or partition keys. Downstream sinks that auto-create tables (Doris, StarRocks, MySQL, ...) therefore lost comments and keys that existed on the real source table.
• Fix approach: resolve the physical table the query maps to (when it can be verified to be a single physical table) and reuse the existing mergeCatalogTable logic to backfill that missing metadata into the query-derived schema, mirroring what already happens when a user configures table_path together with query.
• One-sentence summary: this restores comment/key metadata for query-only JDBC sources without changing row shape, and — after this round's fix — does so only when it can actually verify the query is single-table.

1. Code Change Review

1.1 Core Logic Analysis

This round's diff vs. the version I reviewed before (fe3c866fd..281fe76d1) is scoped exactly to closing Issue 1 from my last review:

// CatalogUtils.java (new)
public static boolean isSinglePhysicalTable(ResultSetMetaData metadata) {
    ...
    TablePath tablePath = getPhysicalTablePath(metadata, 1);
    if (tablePath == null) return false;
    for (int index = 2; index <= columnCount; index++) {
        if (!tablePath.equals(getPhysicalTablePath(metadata, index))) {
            return false;
        }
    }
    return true;
}

// JdbcCatalogUtils.java
static CatalogTable mergeWithUnderlyingTable(
        CatalogTable tableOfQuery,
        boolean singlePhysicalTable,
        UnderlyingTableLoader underlyingTableLoader) {
    if (!singlePhysicalTable) {
        return tableOfQuery;
    }
    ...
}

The singlePhysicalTable boolean is now computed and threaded through at both call sites (catalog-backed path, JdbcCatalogUtils.java:275-282, and the no-catalog fallback path, JdbcCatalogUtils.java:477-484), and AbstractJdbcCatalog/OceanBaseMySqlCatalog/YashanDbCatalog each got a corresponding isSinglePhysicalTableQuery(String) override so every code path that can produce a query-derived CatalogTable is covered.

Key Findings:

• Issue 1 from my previous review (join misattribution via the column-1-only heuristic) is fixed correctly: isSinglePhysicalTable now checks that every selected column's (catalog, schema, table) triple matches column 1's, not just that column 1 resolves to an existing table. SELECT o.id, o.amount, c.name FROM orders o JOIN customers c ... now returns false and the merge is skipped, verified by the new testJoinIsNotDetectedAsSinglePhysicalTable / testMergeWithUnderlyingTableSkippedForMultiTableQuery unit tests.
• Issue 2 from my previous review (missing Javadoc on UnderlyingTableLoader) is also fixed — a one-line Javadoc was added.
• The guard is applied uniformly to catalog-backed sources, the no-catalog fallback, and the Statement-based metadata paths for OceanBase/YashanDB, as the author's summary claims. I traced all three and confirm this.
• New finding (see Issue 1 below): the "every column must match" check is stricter than necessary and now silently disqualifies a common, legitimately single-table shape — a query against one real table that also selects a computed/expression column (e.g. SELECT id, UPPER(name) AS name_upper FROM orders) — because JDBC drivers typically report a blank/null table name for expression columns, which getPhysicalTablePath turns into null, which never equals column 1's table path. This is fail-safe (no worse than pre-PR behavior, since the merge is simply skipped), but it is a real, currently-undocumented-and-untested gap that narrows the feature's applicability.
• New finding (see Issue 2 below): for OceanBase/YashanDB, the new isSinglePhysicalTableQuery override calls Statement.executeQuery(sqlQuery) a second time, in addition to the pre-existing Statement.executeQuery already performed by getTable(sqlQuery) for those two dialects. This actually re-runs the user's query, not just re-derives metadata, which is a new, real cost/side-effect increase specific to these two dialects.

In-depth correctness analysis: The single-table check only affects CatalogTable metadata built during source-table discovery/planning (getTables() → getCatalogTable(...) → mergeWithUnderlyingTable); it has no bearing on the hot read/write path, checkpoint state, or SeaTunnelRow shape. Column order and row content are unaffected in all branches, so recovery/restore compatibility for already-running jobs is unaffected by this round's change, consistent with my previous conclusion.

1.2 Compatibility Impact

Fully compatible for the risk I raised previously — the join-misattribution scenario is now closed, so the "silently attach a wrong PK/constraint to a join result" compatibility risk from my last review is resolved. Row shape and column order are unaffected, as before. Compatibility classification: Fully compatible (no change to APIs, protocols, serialization, or previously-safe historical behavior; the new behavior only ever narrows/skips a feature this same PR is introducing, it never resurrects the old bug once fixed).

1.3 Performance / Side-Effect Analysis

Negligible for the generic (PreparedStatement) path, same conclusion as last round — one extra prepareStatement(...).getMetaData() call at discovery time, not per record.

Not negligible for OceanBase/YashanDB — see Issue 2. Statement.executeQuery() actually executes the query, so table discovery for a query-only OceanBase/YashanDB source now runs the configured SQL twice instead of once. For an expensive query (unbounded scan) or one containing volatile/side-effecting expressions (e.g. SELECT nextval(seq), ... FROM dual), this is a real, not just theoretical, extra cost/side-effect at job-planning time.

1.4 Error Handling and Logging

isSinglePhysicalTable catches SQLException and returns false (treat as "unverified", conservative default), logged at debug level — good defensive default, consistent with the rest of the merge path. isSinglePhysicalTableQuery(AbstractJdbcCatalog, String) in JdbcCatalogUtils similarly catches Exception and defaults to false with a debug log. No exception is swallowed silently without a trace, and nothing here can misreport success.

Issue 1: Location seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/utils/CatalogUtils.java:338-378 (isSinglePhysicalTable / getPhysicalTablePath), and the doc claim at docs/en/connectors/source/Jdbc.md:50 / docs/zh/connectors/source/Jdbc.md:50.

• Problem description: this code sits in the "does the query map to a single physical table" check that gates the entire metadata merge added by this PR. It requires every column to report a non-blank table name that matches column 1's. JDBC drivers commonly report a blank/null table name for computed or expression columns (functions, CASE, casts, literals) because those columns have no single origin column. A perfectly ordinary single-table query such as SELECT id, UPPER(name) AS name_upper FROM orders will have orders reported for id but a blank table name for name_upper, which makes isSinglePhysicalTable return false for the whole query, not just skip merging metadata for that one column.
• Potential risk: the feature this PR adds (restoring comments/PK/constraint/partition-key metadata for query-only sources) will silently not activate for any single-table query that also selects a computed/expression column — a very common shape for query-only sources (that's exactly why users reach for query instead of table_path in the first place: to add casts, renames, and derived columns). It is fail-safe (falls back to the pre-PR behavior, no incorrect metadata), so it is not a correctness/data-loss risk, but it is a functional gap that will surprise users, and it directly contradicts the new doc wording: docs/en/connectors/source/Jdbc.md:50 says "Columns renamed or transformed in the query keep the query-derived definition" (implying the rest of the merge still happens), but the actual code disqualifies the entire merge, not just that one column, when a transformed column has no table origin. There is also no unit or E2E test covering "single real table + one computed column," so this gap is currently untested and undocumented as-is.
• Best improvement: change the check to only compare columns that do report table-origin metadata against column 1, and simply ignore/skip columns with no reported origin (blank table name) rather than treating them as disqualifying. That still safely rejects the join case (multiple real origins reported) while correctly still detecting the single-table + computed-column case. Alternatively, if the stricter "unknown means disqualified" semantics is intentional as an extra-conservative default, then the doc sentence about "transformed columns keep the query-derived definition" needs to be corrected to make clear the entire merge is skipped whenever any column's origin can't be verified, and a regression test for this exact scenario should be added either way.
• Severity: Medium.
• Raised by another reviewer: No.

Issue 2: Location seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oceanbase/OceanBaseMySqlCatalog.java:265-272 and seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/yashandb/YashanDbCatalog.java:234-241.

• Problem description: this sits in the same source-table-discovery path as Issue 1, specifically the dialect-specific override of isSinglePhysicalTableQuery for OceanBase and YashanDB. Both overrides do Statement.createStatement().executeQuery(sqlQuery) to obtain ResultSetMetaData. Both catalogs already do the exact same Statement.executeQuery(sqlQuery) once inside their (pre-existing, unmodified by this PR) getTable(String sqlQuery) override — YashanDB's own comment even documents why: "YashanDB JDBC driver does not implement PreparedStatement.getMetaData()". With this change, discovering a query-only OceanBase/YashanDB source now executes the user's configured SQL twice (once in getTable, once in the new isSinglePhysicalTableQuery) instead of once.
• Potential risk: this is confined to source-table discovery/planning (once per configured query-only table at job start), so it is not a hot-path performance regression, but it is a real, not just theoretical, doubling of query execution cost for these two dialects, and — more importantly — a correctness concern for queries with side-effecting or non-idempotent expressions (sequence nextval, volatile/random functions, or anything that mutates state as a side effect of a SELECT). Re-running such a query a second time purely to check "is this single-table" can silently duplicate that side effect. This is a new behavior introduced by this round's fix, not present before.
• Best improvement: reuse the ResultSetMetaData already obtained by getTable(sqlQuery)'s single execution instead of running the query again. For example, have getTable expose/cache the ResultSetMetaData it already fetched (or restructure so a single execution produces both the schema and the single-table verdict), so the single-table check for these two dialects costs zero extra query executions instead of one full extra execution.
• Severity: Medium.
• Raised by another reviewer: No.

2. Code Quality Assessment

2.1 Coding Standards

Clean, well-scoped diff — the fix is exactly targeted at the previously-raised issues, with no unrelated churn. New public method AbstractJdbcCatalog.isSinglePhysicalTableQuery and the private CatalogUtils.getPhysicalTablePath both have brief Javadoc. UnderlyingTableLoader now has the requested Javadoc.

2.2 Test Coverage and Test Stability

New unit tests directly targeting this round's fix:

• CatalogUtilsTest.testSinglePhysicalTableDetectedFromAllResultColumns and testJoinIsNotDetectedAsSinglePhysicalTable — positive/negative cases for the new isSinglePhysicalTable, both pure mock-based, deterministic.
• JdbcCatalogUtilsTest.testMergeWithUnderlyingTableSkippedForMultiTableQuery — verifies the loader is never invoked and the query-derived table is returned unchanged when singlePhysicalTable=false.
• The existing 6 mergeWithUnderlyingTable boundary tests were all updated to pass the new singlePhysicalTable parameter.

All of these are deterministic pure-function tests with mocked ResultSetMetaData — no sleeps, no shared state, no timing dependency. Test Stability rating: Stable (evidence: CatalogUtilsTest.java new tests use only Mockito stubs and plain assertions; JdbcCatalogUtilsTest.java new/updated tests use in-memory AtomicBoolean flags and synchronous lambdas, no threads or clocks involved).

Test blind spot (this is the same code path as Issue 1 above): there is still no unit or E2E test for "single real table + one computed/expression column," which is exactly the scenario where the new guard over-triggers. I'd recommend adding one alongside the join test, e.g. a ResultSetMetaData mock where column 1 reports orders and column 2 reports a blank table name, asserting the current (arguably too strict) behavior explicitly, so any future change to the semantics here is a deliberate, visible diff rather than an accidental behavior change.

2.3 Documentation Updates

docs/en and docs/zh were both updated this round to reflect the "every column must report the same table" wording and to cross-link the "Query and primary-key caution" section, which is exactly what I asked for last time — nice catch on tightening the wording. However, per Issue 1 above, the sentence "Columns renamed or transformed in the query keep the query-derived definition" is no longer accurate for transformed/expression columns once the new single-table check is in play — it should be updated to state plainly that a query containing a column with no verifiable table origin causes the whole merge to be skipped.

3. Architectural Soundness

3.1 Elegance of the Solution

This round's fix is a precise fix for the specific gap I flagged previously — thread a boolean verdict through the existing merge function rather than reworking the merge logic itself. Good minimal-footprint choice, consistent with the rest of the PR.

3.2 Maintainability

Good — the new isSinglePhysicalTable/getPhysicalTablePath helpers are small, self-contained, and unit-testable without any real JDBC connection.

3.3 Extensibility

The per-dialect isSinglePhysicalTableQuery override pattern mirrors the existing per-dialect getTable(String) override pattern, so it is easy for a future dialect with unusual metadata behavior to plug in correctly. The downside (Issue 2) is that it also inherited the "must actually execute the query" limitation from the pre-existing getTable override for OceanBase/YashanDB without deduplicating the two executions.

3.4 Historical-Version Compatibility

Unchanged from my previous conclusion: row/column structure and order are preserved, so checkpoint/savepoint restore for already-running jobs is unaffected. The risk surface remains confined to newly-derived CatalogTable metadata at discovery/sink-auto-create time.

4. Issue Summary

Number	Issue	Location	Severity
1	"Single physical table" check disqualifies the entire merge whenever any column has no verifiable table origin (e.g. a computed/expression column in an otherwise single-table query), which is both stricter than necessary and now inconsistent with the updated docs' "renamed or transformed columns keep the query-derived definition" claim	CatalogUtils.java:338-378, docs/en/connectors/source/Jdbc.md:50, docs/zh/connectors/source/Jdbc.md:50	Medium
2	New isSinglePhysicalTableQuery override for OceanBase/YashanDB executes the configured query a second time via Statement.executeQuery, on top of the existing execution in getTable(sqlQuery), doubling query-execution cost/side effects for these two dialects during table discovery	OceanBaseMySqlCatalog.java:265-272, YashanDbCatalog.java:234-241	Medium

For the record, both High/Low issues from my previous review (#5029372846) are resolved in this head:

• Previous Issue 1 (High, join misattribution) — fixed by the new isSinglePhysicalTable guard, verified by testJoinIsNotDetectedAsSinglePhysicalTable.
• Previous Issue 2 (Low, missing Javadoc) — fixed, UnderlyingTableLoader now has a one-line Javadoc.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed

   • None carried over from my previous review; the previous High-severity blocker (join misattribution) is resolved.

2. Recommended fixes — non-blocking

   • Issue 1 (Medium): relax the single-table check so columns with no reported table origin (computed/expression columns) are ignored rather than disqualifying, or at minimum correct the doc sentence that currently contradicts the actual (stricter) behavior, and add a regression test for a single-table query with a computed column.
   • Issue 2 (Medium): avoid the double query execution for OceanBase/YashanDB by reusing the ResultSetMetaData already obtained inside getTable(sqlQuery) instead of issuing a second Statement.executeQuery in isSinglePhysicalTableQuery.

Overall assessment: this round's fix correctly and cleanly closes the join-misattribution risk I flagged previously, with solid, deterministic unit-test coverage for the join case specifically. In the process of making the check safe against joins, it became slightly too conservative for legitimate single-table queries that mix a real column with a computed/expression column, and for two Statement-based dialects it introduced a genuine (if narrow) double-execution side effect. Neither of these is a regression versus the current dev baseline (both degrade gracefully rather than producing incorrect output), so I would not block merge on them, but I'd like to see them addressed, ideally before merge given how small the fixes are, or as an immediate fast-follow.

On CI: the current Build check on this fork run (https://github.com/yzeng1618/seatunnel-dev/actions/runs/33033348912) shows 4 failing jobs, none of which touch a file this PR changed:

• updated-modules-integration-test-part-6 (both JDK 8 and 11) fails identically in JdbcDmSaveModeIT#testJdbcDb with a DM (Dameng) driver DMException: Error in line: 4 during the sink-write phase of a Flink save-mode job — this PR does not touch any DM-related catalog file, and the DM catalog/DDL tests in the same class (testCreateIndex, testCatalog, testCatalogWithCatalogUtils) all pass, which is consistent with this being a pre-existing environment/driver issue with the DM test container rather than something introduced here.
• updated-modules-integration-test-part-2 (JDK 8) fails on connector-hudi-e2e with Could not resolve dependencies ... Connection reset while downloading org.testcontainers:minio:1.19.1 from Maven Central — a transient network failure unrelated to this PR.
• doris-connector-it (JDK 11) fails a single assertion in DorisErrorIT.testDoris (expected: <true> but was: <false>) — this PR does not touch the Doris connector at all.

I also checked base...head and this branch is currently 10 commits behind dev (diverged, ahead_by=2, behind_by=10). Given the failures above look environmental/unrelated to this diff, the lowest-cost next step would be to sync with the latest dev and rerun the Build to rule out any interference from the base drift; if the same three failures persist after that, they're worth reporting separately as they don't appear related to this PR's changes.

Thanks again for addressing the review so promptly and thoroughly — the fix and the new tests are exactly on target for what I raised. Happy to take another look once the two follow-up points above are addressed (or sooner, if you'd rather discuss trade-offs on Issue 1's exact semantics first).

[SEZ9]

Thanks for working on this. I reviewed the latest head 281fe76d1f76.

What this PR fixes

• User pain: JDBC sources configured with query only (no table_path) produce a schema derived purely from ResultSetMetaData, so sinks that auto-create tables (Doris, StarRocks, MySQL) lose column/table comments, primary/constraint keys, and partition keys (#11969, historical #8329).
• Fix approach: when JDBC origin metadata reports every result column as coming from the same physical table, resolve that table (via CatalogUtils.isSinglePhysicalTable plus catalog overrides) and merge its metadata into the query-derived table through the existing mergeCatalogTable path, falling back to the query-derived table on any failure.
• One-line summary: the merge logic and single-table guard are sound in shape and well unit-tested, but the YashanDB override closes the shared cached connection (poisoning the catalog's connection map), the OceanBase/YashanDB overrides re-execute the full unbounded user query at planning time, the verified origin TablePath diverges from the TablePath actually loaded for the merge, and all failure paths are swallowed at DEBUG — these need fixing before merge.

Runtime chain I rechecked

Catalog-backed path (query-only source table):
  JdbcCatalogUtils.getCatalogTable(tableConfig, jdbcCatalog)  JdbcCatalogUtils.java:272
    -> jdbcCatalog.getTable(tableConfig.getQuery())  JdbcCatalogUtils.java:275   (query-derived CatalogTable)
    -> JdbcCatalogUtils.isSinglePhysicalTableQuery(jdbcCatalog, query)  JdbcCatalogUtils.java:487
         -> AbstractJdbcCatalog.isSinglePhysicalTableQuery(sqlQuery)  AbstractJdbcCatalog.java:695   (prepareStatement on cached defaultUrl connection; query is prepared a second time)
              -> CatalogUtils.isSinglePhysicalTable(statement.getMetaData())  CatalogUtils.java:342
                   -> CatalogUtils.getPhysicalTablePath(metadata, index)  CatalogUtils.java:368   (catalog/schema/table origin per column; blank table name => null => unverified)
         [override] OceanBaseMySqlCatalog.isSinglePhysicalTableQuery  OceanBaseMySqlCatalog.java:266   (statement.executeQuery(sqlQuery) — full unbounded execution just for metadata)
         [override] YashanDbCatalog.isSinglePhysicalTableQuery  YashanDbCatalog.java:235   (try-with-resources CLOSES getConnection(defaultUrl) — the shared cached connection stays closed in connectionMap)
    -> JdbcCatalogUtils.mergeWithUnderlyingTable(tableOfQuery, singlePhysicalTable, loader)  JdbcCatalogUtils.java:508
         -> tableOfQuery.getTableId().toTablePath()  JdbcCatalogUtils.java:518   (NOTE: path taken from query-derived TableIdentifier, NOT from the origin TablePath verified in isSinglePhysicalTable)
         -> loader.load(tablePath)  JdbcCatalogUtils.java:527
              -> jdbcCatalog.tableExists(tablePath) ? jdbcCatalog.getTable(tablePath) : null  JdbcCatalogUtils.java:279
         -> JdbcCatalogUtils.mergeCatalogTable(tableOfPath, tableOfQuery)  JdbcCatalogUtils.java:531   (existing merge: guards by column name + SqlType)
         -> catch (Exception) -> log.debug + return tableOfQuery  JdbcCatalogUtils.java:536   (silent fallback)

No-catalog fallback path:
  JdbcCatalogUtils.getCatalogTable(connection, tableConfig, jdbcDialect)  JdbcCatalogUtils.java:475
    -> jdbcDialect.getResultSetMetaData(connection, query)  JdbcCatalogUtils.java:479   (single metadata fetch, reused — the pattern the catalog path could adopt)
    -> CatalogUtils.isSinglePhysicalTable(resultSetMetaData)  JdbcCatalogUtils.java:484
    -> JdbcCatalogUtils.mergeWithUnderlyingTable(...)  JdbcCatalogUtils.java:482
         -> CatalogUtils.getCatalogTable(connection, tablePath, jdbcDialect)  JdbcCatalogUtils.java:485

Findings

Issue 1: YashanDbCatalog.isSinglePhysicalTableQuery closes the shared cached connection from getConnection(defaultUrl), poisoning the catalog's connection cache and guaranteeing the new merge step always fails for YashanDB

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/yashandb/YashanDbCatalog.java:236
• Why it matters: - Root cause: AbstractJdbcCatalog.getConnection(defaultUrl) returns a connection cached in the catalog's connection map — which is exactly why the new AbstractJdbcCatalog.isSinglePhysicalTableQuery and the OceanBase override deliberately do NOT put the connection in try-with-resources and only close the Statement/ResultSet. The YashanDB override, however, declares Connection defaultConnection = getConnection(defaultUrl) inside try-with-resources, so the cached connection is closed when the block exits while the closed instance remains in the connection map.
• Impact: the subsequent tableExists(tablePath) / getTable(tablePath) calls in JdbcCatalogUtils.mergeWithUnderlyingTable reuse the same cached (now closed) connection, so the metadata merge always fails for YashanDB — silently, because the failure is only logged at DEBUG — and any later catalog operation on this URL also fails on a closed connection.
• Suggested fix: follow the pattern used in AbstractJdbcCatalog.isSinglePhysicalTableQuery and the OceanBase override — keep the shared connection out of try-with-resources and close only the Statement/ResultSet.
• Evidence: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/AbstractJdbcCatalog.java:696 Connection defaultConnection = getConnection(defaultUrl);; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:280 jdbcCatalog.tableExists(tablePath); seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/yashandb/YashanDbCatalog.java:238 ResultSet rs = stmt.executeQuery(sqlQuery)) {
• Suggested fix: getConnection(defaultUrl) returns the connection cached in AbstractJdbcCatalog's connection map, so putting it in try-with-resources closes the shared cached connection. The subsequent tableExists(tablePath) / getTable(tablePath) calls in JdbcCatalogUtils.mergeWithUnderlyingTable reuse that same cached connection, so the merge will always fail here (silently, at debug level), and any later catalog operation on this URL will fail on a closed connection. Please follow the pattern used in AbstractJdbcCatalog.isSinglePhysicalTableQuery and the OceanBase override: keep the connection open and only close the Statement/ResultSet.
• Severity: High

Issue 2: OceanBase/YashanDB overrides execute the full user query with no row limit just to read metadata, risking OOM/long stalls on large tables

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oceanbase/OceanBaseMySqlCatalog.java:269
• Why it matters: - Root cause: in the catalog-backed path, JdbcCatalogUtils.getCatalogTable first calls jdbcCatalog.getTable(tableConfig.getQuery()) and then separately calls isSinglePhysicalTableQuery(jdbcCatalog, tableConfig.getQuery()). For OceanBase and YashanDB that second call is statement.executeQuery(sqlQuery) on a plain Statement with no setMaxRows or LIMIT 0 wrapper — the arbitrary user query is fully executed again just to read ResultSetMetaData.
• Impact: for a query-only source over a large table (e.g. SELECT * FROM huge_table), MySQL-protocol drivers buffer the whole result set client-side by default, so this planning-time metadata probe can OOM or stall the job before any read starts — and it is a second full execution on top of getTable(sqlQuery).
• Suggested fix: call statement.setMaxRows(1) before executeQuery, or reuse the metadata already fetched by getTable(sqlQuery) instead of executing the query again.
• Evidence: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oceanbase/OceanBaseMySqlCatalog.java:268 try (Statement statement = connection.createStatement();; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/yashandb/YashanDbCatalog.java:238 ResultSet rs = stmt.executeQuery(sqlQuery)) {; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oceanbase/OceanBaseMySqlCatalog.java:269 ResultSet resultSet = statement.executeQuery(sqlQuery)) {
• Suggested fix: This executes the user's full query with no row cap only to inspect ResultSetMetaData. With MySQL-protocol drivers the whole result set is buffered client-side by default, so SELECT * FROM huge_table in query-only mode can OOM or stall the job at planning time — and this is a second full execution on top of getTable(sqlQuery). Please call statement.setMaxRows(1) before executeQuery, or reuse the metadata already fetched by getTable(sqlQuery).
• Severity: High

Issue 3: The metadata-verified origin TablePath is discarded; the merge loads whatever TablePath the query-derived TableIdentifier happens to carry, which can differ from the verified origin table and merge PK/comments from the wrong table

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:520
• Why it matters: - Root cause: CatalogUtils.isSinglePhysicalTable resolves the actual origin TablePath from getCatalogName/getSchemaName/getTableName per column but returns only a boolean, throwing the resolved path away. mergeWithUnderlyingTable then re-derives the path from tableOfQuery.getTableId().toTablePath(), which is populated by a completely separate code path (jdbcCatalog.getTable(query) in the catalog branch), so nothing guarantees the two paths agree.
• Impact: if the query-derived identifier resolves to a same-named table in a different database/schema, the merge injects that table's primary key, constraint keys, and comments into the query result, silently changing sink upsert and auto-create semantics based on the wrong table's metadata.
• Suggested fix: make isSinglePhysicalTable return the resolved Optional<TablePath> and load exactly that table for the merge, or at minimum verify it equals the query-derived path before merging.
• Evidence: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:520 TablePath tablePath = tableId.toTablePath();; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:517 TableIdentifier tableId = tableOfQuery.getTableId();; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:521 TablePath tablePath = tableId.toTablePath();
• Suggested fix: The path we load here comes from tableOfQuery.getTableId(), but the single-table verification in CatalogUtils.isSinglePhysicalTable resolved the actual origin TablePath from the driver metadata and then discarded it. Nothing guarantees the two agree — if the query-derived identifier resolves to a same-named table in another database/schema, we'd merge that table's primary key and constraint keys into the query result and change sink upsert/auto-create semantics. Please make isSinglePhysicalTable return the resolved Optional<TablePath> and load exactly that table (or verify it equals this path before merging).
• Severity: Medium

Issue 4: For drivers that report only schema (no catalog), the resolved TablePath has a null database name, so tableExists/getTable throws and the merge silently never applies

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:280
• Why it matters: - Root cause: getPhysicalTablePath builds TablePath.of(defaultIfBlank(catalog, null), defaultIfBlank(schema, null), tableName), and the guard in mergeWithUnderlyingTable accepts the path when either database OR schema is non-blank. Schema-based databases (Oracle, DM, and PostgreSQL-style drivers) typically return a blank getCatalogName, yielding a TablePath with a null database name.
• Impact: the catalog-path loader then calls jdbcCatalog.tableExists(tablePath) with databaseName == null, which throws inside databaseExists(null); the broad catch converts that into a silent DEBUG-level fallback, so the metadata merge effectively never runs for these dialects despite what the docs promise.
• Suggested fix: default a null database name to jdbcCatalog.getDefaultDatabase() before the tableExists/getTable lookup, and add coverage for the schema-only TablePath case in JdbcCatalogUtilsTest.
• Evidence: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:522 || (StringUtils.isBlank(tablePath.getDatabaseName()); seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:280 jdbcCatalog.tableExists(tablePath)
• Suggested fix: For schema-based dialects the driver usually reports a blank getCatalogName, so the guard above lets through a TablePath whose database name is null. AbstractJdbcCatalog.tableExists then goes through databaseExists(null), which throws — the broad catch turns that into a silent fallback, so the metadata merge effectively never runs for Oracle/PG-style catalogs despite the docs. Please default a null database name to jdbcCatalog.getDefaultDatabase() before this lookup, and cover the schema-only TablePath case in JdbcCatalogUtilsTest.
• Severity: Medium

Issue 5: User-facing behavior change (automatic metadata merge for query-only sources) is documented in the connector docs but not recorded in the changelog / incompatible-changes notes

• Location: docs/en/connectors/source/Jdbc.md:50
• Why it matters: - Root cause: this PR silently changes what a query-only JDBC source emits — the produced CatalogTable now carries the underlying table's comments, primary key, constraint keys, and partition keys, without any new option to opt out. The PR checklist item for incompatible-changes.md is unchecked and no changelog entry is included.
• Impact: users upgrading with existing query-only jobs against auto-create sinks (Doris, StarRocks, MySQL) will get differently-structured sink tables after upgrade, with no release-notes signal warning them of the change or explaining how to retain the previous behavior.
• Suggested fix: add an entry to incompatible-changes.md / the release notes describing the new merge behavior, and document whether users can keep the previous query-derived-only schema.
• Evidence: docs/zh/connectors/source/Jdbc.md:50 当只配置 query，且 JDBC 报告所有结果列均来自同一张物理表（例如 SELECT * FROM db.table）时，SeaTunnel 会根据查询结果元数据解析该表，并把底表的元数据（字段注释、表注释、主键、约束键、分区键）合并进查询推导出的 schema
• Suggested fix: This paragraph documents the new behavior well, but the change is applied automatically to every existing query-only job with no opt-out — auto-created sink tables (Doris/StarRocks/MySQL) will gain a primary key and comments they did not have before the upgrade. Please also record this in incompatible-changes.md / the release notes (the PR checklist item is still unchecked), and state whether users have any way to keep the previous query-derived-only behavior.
• Severity: Medium

Issue 6: Query-only pipelines gain a merged primary key on upgrade, changing split planning and sink auto-create DDL; incompatible-changes.md is not updated

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:276
• Why it matters: - Root cause: the new merge at JdbcCatalogUtils.java:276-282 injects the underlying table's primary key, constraint keys, partition keys, and options into every query-only CatalogTable that resolves to a single physical table — unconditionally, with no option to opt out.
• Impact: existing query-only jobs that previously produced a schema without a PK may now be dynamically split by the merged PK (the query wrapped in a range-filtered subquery, multiple parallel reads), changing read parallelism and ordering; auto-create sinks also emit different DDL (PK, comments, options) than before the upgrade.
• Suggested fix: record this upgrade-visible behavior change in incompatible-changes.md (the checklist item is unchecked) and consider adding an opt-out option for users whose queries or manually-tuned sink tables depend on the old behavior.
• Evidence: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:280 jdbcCatalog.tableExists(tablePath); docs/en/connectors/source/Jdbc.md:50 When only queryis configured and JDBC reports every result column as originating from the same physical table (for exampleSELECT * FROM db.table), SeaTunnel resolves that table from the query result metadata and merges its metadata — column and table comments, primary key, constraint keys, and partition keys — into the query-derived schema
• Suggested fix: This changes semantics for every existing query-only pipeline on upgrade: a merged primary key now feeds split-key inference (jobs that ran as a single split can become PK-range-split parallel reads with the query wrapped in a subquery), and auto-create sinks emit different DDL (PK/comments/options). That's intended, but it is upgrade-visible — please add an entry to incompatible-changes.md (the checklist item is unchecked) and consider an opt-out option for users whose queries or manually-tuned sink tables depend on the old behavior.
• Severity: Medium

Issue 7: Catalog path prepares/executes the query twice; the no-catalog path already shows how to reuse a single ResultSetMetaData

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:278
• Why it matters: - Root cause: in the catalog-backed path, jdbcCatalog.getTable(tableConfig.getQuery()) fetches the query metadata once, then isSinglePhysicalTableQuery(jdbcCatalog, tableConfig.getQuery()) prepares (and for OceanBase/YashanDB fully executes) the very same query a second time. In contrast, the fallback path in the same file correctly reuses one ResultSetMetaData for both getCatalogTable and CatalogUtils.isSinglePhysicalTable.
• Impact: doubled round trips and query planning cost per source table at job startup; two separate metadata fetches also open a race window where the underlying table can change between them.
• Suggested fix: expose a catalog method that returns both the CatalogTable and the single-table flag from one metadata fetch, mirroring the no-catalog path's single-fetch pattern.
• Evidence: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:278 isSinglePhysicalTableQuery(jdbcCatalog, tableConfig.getQuery()),; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:484 CatalogUtils.isSinglePhysicalTable(resultSetMetaData),
• Suggested fix: getTable(query) on line 275 already fetched the query's metadata, and this call prepares/executes the same query again (a full re-execution on OceanBase/YashanDB). The no-catalog path below reuses a single ResultSetMetaData for both the schema and isSinglePhysicalTable — can we do the same here, e.g. a catalog method returning both the CatalogTable and the single-table flag from one metadata fetch? Besides the cost, two separate fetches also open a race where the table changes between them.
• Severity: Medium

Issue 8: All fallback paths swallow exceptions at DEBUG level, making the advertised metadata-preservation feature silently no-op with no diagnosable signal

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:493
• Why it matters: - Root cause: both the isSinglePhysicalTableQuery wrapper and the loader failure branch of mergeWithUnderlyingTable catch broad Exception and log only at log.debug, then fall back to the query-derived table.
• Impact: the PR documents (en/zh docs) that comments/PK/constraint keys are now preserved in query-only mode, so users will rely on it. When it fails at runtime — closed connection (see the YashanDB finding), permission denied on metadata queries, unsupported driver metadata — users just get a sink table without the promised metadata and no clue why at default log levels.
• Suggested fix: raise these fallback logs to warn/info and include the table path and exception message so the silent fallback becomes diagnosable.
• Evidence: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:494 "Failed to verify whether query maps to a single physical table, skip merging underlying table metadata",; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:537 log.debug("Failed to load the underlying table {} of query, use the query-derived table directly", tablePath, e);; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:493 log.debug(
• Suggested fix: The docs added in this PR promise that comments/PK/constraint keys are preserved in query-only mode, but every failure path here (and in mergeWithUnderlyingTable) is swallowed at debug. When merging fails (permissions, driver quirks, the closed-connection issue in YashanDB), users just get a sink table without comments and no clue why at default log levels. Please raise these to warn/info with the table path and exception message so the fallback is diagnosable.
• Severity: Medium

Review conclusion

Conclusion: can merge after the blocking items are fixed

1. Blocking items

• Issue 1: YashanDbCatalog.isSinglePhysicalTableQuery closes the shared cached connection from getConnection(defaultUrl), poisoning the catalog's connection cache and guaranteeing the new merge step always fails for YashanDB
• Issue 2: OceanBase/YashanDB overrides execute the full user query with no row limit just to read metadata, risking OOM/long stalls on large tables

2. Remaining follow-ups

• Issue 3: The metadata-verified origin TablePath is discarded; the merge loads whatever TablePath the query-derived TableIdentifier happens to carry, which can differ from the verified origin table and merge PK/comments from the wrong table
• Issue 4: For drivers that report only schema (no catalog), the resolved TablePath has a null database name, so tableExists/getTable throws and the merge silently never applies
• Issue 5: User-facing behavior change (automatic metadata merge for query-only sources) is documented in the connector docs but not recorded in the changelog / incompatible-changes notes
• Issue 6: Query-only pipelines gain a merged primary key on upgrade, changing split planning and sink auto-create DDL; incompatible-changes.md is not updated
• Issue 7: Catalog path prepares/executes the query twice; the no-catalog path already shows how to reuse a single ResultSetMetaData
• Issue 8: All fallback paths swallow exceptions at DEBUG level, making the advertised metadata-preservation feature silently no-op with no diagnosable signal

Thanks for the updates so far. Happy to discuss the remaining points.

[DanielLeens]

Thanks for the thorough follow-up review, @SEZ9 — this is a genuinely useful second pass on top of mine.

Issue 1 (YashanDB isSinglePhysicalTableQuery closing the shared cached connection via try-with-resources on getConnection(defaultUrl)) is a real catch I missed in both of my rounds. I only checked that the pattern mirrored AbstractJdbcCatalog.isSinglePhysicalTableQuery/OceanBaseMySqlCatalog's "don't close the shared connection" convention at a glance, not that YashanDbCatalog actually followed it — it doesn't, and closing the cached defaultUrl connection poisons the connection map for every later tableExists/getTable call on that catalog instance, not just this new code path. Agreed this is a High-severity blocker.

On Issue 2 (OceanBase/YashanDB executing the full query a second time): this overlaps with my own Issue 2/recommended-fix from the previous round — I had it as a doubled-execution cost/side-effect concern and kept it non-blocking since it degrades to "slower, not wrong." Your framing (no setMaxRows/row cap on an arbitrary user query, so a SELECT * FROM huge_table can buffer client-side and OOM at planning time) is the sharper and more production-relevant version of the same root cause, and combined with Issue 1 landing in the same connection-handling code, I agree this should move to a blocker alongside Issue 1 rather than stay a fast-follow.

Issues 3 and 4 (verified origin TablePath discarded in favor of the query-derived TableIdentifier's path, and null database name for schema-only dialects swallowing into a silent tableExists failure) are both new findings I didn't trace — the TablePath-divergence one in particular is a correctness gap worth closing before merge, not just a follow-up, since it can attach the wrong table's PK/constraints under a same-named-table-in-another-schema collision.

Issues 5/6 (incompatible-changes.md) match what I flagged as Medium/non-blocking last round — no disagreement there, just confirming alignment so @yzeng1618 doesn't need to reconcile two separate asks.

Issue 7 is the same double-execution root cause as Issue 2 / my prior finding — agreed, one fix should close both.

Issue 8 (DEBUG-level swallowing on every fallback path) is a fair catch given how much silent-failure surface this PR just added; I'd raise at least the loader-failure branch to warn with the table path included.

@yzeng1618 — given Issue 1 and Issue 2 are now assessed as blocking (connection-pool poisoning plus an unbounded second execution on the same code path), please fold the fixes for those together with Issues 3/4/7/8 in the next commit. I'll hold off on a fresh full review until that lands, then re-review the whole diff against the new head rather than just the delta, since the connection-handling fix may touch the same lines as the double-execution fix.

[DanielLeens]

Re-review acknowledgment: I re-reviewed the current head 227f4bf0033e (a no-op merge of upstream/dev on top of 9e057517bf54) against my last formal review round on 281fe76d1f76. Between those two points, 9e057517bf54 ("Address review findings on query-only metadata merge") is the only substantive commit, and it closes out both my own previous Issue 1/Issue 2 and all eight issues @SEZ9 raised in the CHANGES_REQUESTED review posted after my last round — including the two (connection-cache poisoning, unbounded second query execution) that I already agreed in my 2026-08-29 follow-up comment should be treated as blockers. I re-traced the full call chain from scratch rather than only the delta, and re-verified the previously-approved scope (docs wording, the merge/guard shape, the join-detection logic) is functionally intact aside from the refactor described below.

What Problem Does This PR Solve?

• User pain point: a JDBC source defined purely with query (no table_path) derives its schema only from ResultSetMetaData, which carries no column/table comments, primary/constraint keys, or partition keys — sinks that auto-create tables (Doris, StarRocks, MySQL, ...) lose that metadata.
• Fix approach: when JDBC result metadata verifies every selected column originates from the same physical table, resolve that table and merge its metadata into the query-derived schema via the existing mergeCatalogTable path, exactly as if table_path had been configured alongside query.
• One-sentence summary: this round's commit fixes a real connection-cache-poisoning bug and an unbounded-query-execution risk that @SEZ9 found in the previous head, threads the metadata-verified table path through the merge instead of re-deriving a possibly-different one, defaults a missing database name before the catalog lookup, raises silent-fallback logging from debug to warn/info, and records the behavior change in incompatible-changes.md — all eight of @SEZ9's issues plus my own two are addressed.

1. Code Change Review

1.1 Core Logic Analysis

The core structural change this round is replacing the two-call shape (getTable(query) then a separate isSinglePhysicalTableQuery(...)) with a single resolveQueryTable(String) that returns both the query-derived CatalogTable and the verified single-table origin path from one metadata fetch:

// AbstractJdbcCatalog.java:690-105 (new)
public QueryTableResolution resolveQueryTable(String sqlQuery) throws SQLException {
    CatalogTable tableOfQuery = getTable(sqlQuery);
    Optional<TablePath> singlePhysicalTablePath = Optional.empty();
    try {
        Connection defaultConnection = getConnection(defaultUrl);
        try (PreparedStatement statement = defaultConnection.prepareStatement(sqlQuery)) {
            singlePhysicalTablePath = CatalogUtils.getSinglePhysicalTablePath(statement.getMetaData());
        }
    } catch (Exception e) {
        log.warn("Failed to verify whether the query maps to a single physical table, ...", e);
    }
    return new QueryTableResolution(tableOfQuery, singlePhysicalTablePath);
}

OceanBaseMySqlCatalog and YashanDbCatalog each override resolveQueryTable (not just the old isSinglePhysicalTableQuery hook) so their single Statement.executeQuery(sqlQuery) produces both the schema and the origin path in one execution, and getTable(String) on both dialects now just delegates to resolveQueryTable(sqlQuery).getTable(). JdbcCatalogUtils.mergeWithUnderlyingTable now takes the verified Optional<TablePath> directly as a parameter instead of re-deriving a path from tableOfQuery.getTableId().

Verifying the previously-flagged issues are actually fixed (I traced each one against the new code, not just the commit message):

• @SEZ9 Issue 1 (High) — YashanDB closing the shared cached connection. Fixed. YashanDbCatalog.resolveQueryTable now does Connection defaultConnection = getConnection(defaultUrl); outside the try-with-resources, with only Statement stmt inside it (YashanDbCatalog.java:206-207), and carries an explicit Javadoc warning that the connection "is cached and shared, so it must not be closed here." This is exactly the pattern AbstractJdbcCatalog/OceanBaseMySqlCatalog already used and that was previously missing here. Confirmed by re-reading the full method body, not just the diff hunk.
• @SEZ9 Issue 2 (High, which I had independently as a non-blocking Medium last round) — unbounded second query execution. Fixed via two independent changes that together close it: (a) the resolveQueryTable refactor means OceanBase/YashanDB now execute the query only once, not twice (this also closes @SEZ9 Issue 7, the same root cause); (b) both overrides now call statement.setMaxRows(1) / stmt.setMaxRows(1) before executeQuery (OceanBaseMySqlCatalog.java:236, YashanDbCatalog.java:208), capping the row count instead of letting an arbitrary SELECT * FROM huge_table run unbounded. See Issue 1 below for one residual caveat on this specific fix.
• @SEZ9 Issue 3 (Medium) — verified origin path discarded in favor of the query-derived identifier's path. Fixed. mergeWithUnderlyingTable now receives queryTableResolution.getSinglePhysicalTablePath() directly as a parameter and loads exactly that path (JdbcCatalogUtils.java:390-401, 442-449) — it no longer re-derives a path from tableOfQuery.getTableId(). Verified by the new testMergeWithUnderlyingTableUsesVerifiedOriginTablePath test, which builds a query table with identifier database-y/table-y but passes a verified origin path of database-x/table-x, and asserts the loader is invoked with database-x/table-x — i.e. the same-named-table-in-another-schema collision @SEZ9 described can no longer happen.
• @SEZ9 Issue 4 (Medium) — null database name for schema-only dialects breaking tableExists. Fixed for the catalog-backed path via the new withDefaultDatabase(TablePath, String) helper (JdbcCatalogUtils.java:425-431), applied before the tableExists/getTable lookup (JdbcCatalogUtils.java:396-397), with three new unit tests covering "database missing → filled from catalog default", "database already resolved → left alone", and "no catalog default configured → path unchanged". I separately checked the no-catalog fallback path (CatalogUtils.getCatalogTable(Connection, TablePath, JdbcDialect) at JdbcCatalogUtils.java:489) does not need the same fix: it flows into DatabaseMetaData-based column lookups where a null catalog argument means "don't narrow by catalog" per the JDBC spec, which is the correct behavior for schema-only dialects — unlike AbstractJdbcCatalog.tableExists, which explicitly requires a non-blank database name. So the fix is correctly scoped to the one path that actually needed it.
• @SEZ9 Issues 5/6 (Medium) — no incompatible-changes.md entry for the upgrade-visible behavior change. Fixed. Both docs/en/introduction/concepts/incompatible-changes.md and the docs/zh counterpart gained a new entry under "JDBC Connector" that explicitly documents: unaffected checkpoint/row-shape compatibility, that auto-create sinks will now emit comments/PK/keys they didn't before, and — matching Issue 6 specifically — that "a query-only source without a configured partition_column may now infer a split column from the merged primary key, turning a previously single-split read into parallel range splits," plus the workaround (pre-create the sink table and/or set partition_column explicitly).
• @SEZ9 Issue 8 (Medium) — silent debug-level fallback logging. Fixed. resolveQueryTable's catch block, CatalogUtils.getSinglePhysicalTablePath's catch block, and mergeWithUnderlyingTable's loader-failure catch block are now all log.warn(...) (previously log.debug), and the loader-failure log includes the table path and the exception. A new log.info("Merged the metadata of underlying table {} into the query-derived table", tablePath) was also added on the success path, which is a nice diagnosability improvement beyond what was asked.
• My own previous-round Issue 1 (Medium) — the all-or-nothing single-table check disqualifying a single real table with a computed/expression column, contradicting the doc wording. Resolved via the documentation-correction branch of the alternative I offered last round (rather than relaxing the check itself, which remains intentionally strict). docs/en/connectors/source/Jdbc.md:50 and the zh counterpart now read: "a computed or expression column ... usually reports no origin metadata, which leaves the whole query unverifiable — the merge is then skipped entirely, not only for that column," which now accurately matches the code, and the new testExpressionColumnKeepsSingleTableUnverified unit test pins this exact behavior down as a deliberate, visible test rather than an implicit one. I'm satisfied with this resolution — an all-or-nothing check is the safer default, and it's now honestly documented.
• My own previous-round Issue 2 (Medium) — OceanBase/YashanDB double execution. Superseded by/folded into the fix for @SEZ9 Issue 2/7 above (same root cause, now fixed with a single execution).

In-depth correctness analysis: As with both of my previous rounds, this refactor only affects CatalogTable metadata built during source-table discovery/planning (getCatalogTable(...) → resolveQueryTable/mergeWithUnderlyingTable); it does not touch the hot read/write path, SeaTunnelRow shape, or checkpoint/savepoint state. I re-confirmed this by checking that no changed method is reachable from the reader/writer task lifecycle, only from catalog/table-discovery code paths invoked at job planning time.

1.2 Compatibility Impact

Fully compatible with respect to the risks raised in both previous rounds — the connection-poisoning bug (which would have made YashanDB's entire catalog unusable after the first query-only table discovery, not just this new feature) and the double/unbounded-execution risk are both closed. Row shape and column order remain unaffected in all branches. The upgrade-visible behavior change (query-only sources may now carry a PK/comments they didn't have before, potentially changing split planning and sink auto-create DDL) is now properly disclosed in incompatible-changes.md as required. Compatibility classification: Fully compatible for already-running jobs' checkpoint/restore and row semantics; a disclosed, intentional behavior change for newly-planned query-only jobs, consistent with what both prior rounds concluded and now properly documented.

1.3 Performance / Side-Effect Analysis

Substantially improved from the previous head:

• OceanBase/YashanDB: previously 2 full query executions per query-only table at discovery time (both unbounded); now 1 execution, capped with setMaxRows(1).
• Generic (PreparedStatement) path: unchanged — one extra prepareStatement(...).getMetaData() metadata-only probe at discovery time, no execution.

One residual point (see Issue 1 below): setMaxRows(1) caps the ResultSet the driver returns, but for MySQL-protocol drivers running in their default (non-streaming) fetch mode, the documented behavior is that the driver still buffers the query's full result set from the server into client memory before truncating to the max-rows limit, unless streaming/cursor fetch is explicitly enabled. Since OceanBase's driver is MySQL-protocol-compatible, SELECT * FROM huge_table at discovery time may still transfer/buffer the full result client-side even with setMaxRows(1) set. This is a real, driver-specific caveat, not a hard fact I can confirm without running the OceanBase driver directly, so I'm not treating it as a blocker — the fix is a genuine improvement (bounded object count, single execution, and it does prevent unbounded row processing even if not unbounded row transfer) and is exactly what @SEZ9 suggested as sufficient. But it's worth a follow-up look given the OOM concern that motivated the fix in the first place.

1.4 Error Handling and Logging

All fallback paths now log at warn with contextual information (table path, exception) instead of debug, addressing @SEZ9 Issue 8. Nothing is swallowed without a trace, and no failure path can silently report success.

Issue 1 — Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oceanbase/OceanBaseMySqlCatalog.java:236, seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/yashandb/YashanDbCatalog.java:208.

• Problem description: Statement.setMaxRows(1) limits how many rows the returned ResultSet exposes, but for MySQL-protocol JDBC drivers running with the default (non-streaming, fetchSize=0) statement configuration, the well-documented driver behavior is that the entire result set is still fetched from the server into client memory in one round trip; setMaxRows truncates the client-side buffer afterward rather than limiting what's transferred or materialized. OceanBase's JDBC driver is MySQL-protocol-compatible, so the same caveat plausibly applies there (I have not run the OceanBase driver directly to confirm this either way, so flag it as a driver-behavior risk to verify rather than a confirmed bug).
• Potential risk: if this caveat holds for the OceanBase driver, a query-only OceanBase source over an unbounded query (e.g. SELECT * FROM huge_table) at job-planning time could still transfer/buffer the full result set client-side even with the cap in place — the exact OOM/stall scenario @SEZ9's fix was meant to close, just less likely to trigger now (one execution instead of two).
• Best improvement: if confirmed, wrap the metadata probe as SELECT * FROM (<sqlQuery>) t LIMIT 1 (server-side limit) instead of relying on client-side setMaxRows, or enable streaming fetch (setFetchSize(Integer.MIN_VALUE) for MySQL-family drivers) before executing. If the OceanBase/YashanDB drivers are confirmed to honor setMaxRows as a genuine server-side limit (some MySQL-protocol driver forks do optimize this), this can be closed as a non-issue with a one-line note in the Javadoc confirming it.
• Severity: Medium (real improvement already landed; this is about closing the gap all the way, not a regression versus the previous head).
• Raised by another reviewer: No.

2. Code Quality Assessment

2.1 Coding Standards

Clean, well-scoped diff exactly targeting the previously-raised issues, no unrelated churn. New/changed public surface (AbstractJdbcCatalog.resolveQueryTable, QueryTableResolution, CatalogUtils.getSinglePhysicalTablePath, JdbcCatalogUtils.withDefaultDatabase) all carry Javadoc explaining purpose and, where relevant (YashanDB), a non-obvious constraint ("must not be closed here"). I also verified there are no stale references left to the old isSinglePhysicalTable/isSinglePhysicalTableQuery method names anywhere in connector-jdbc — the rename to getSinglePhysicalTablePath/resolveQueryTable is complete and consistent across all four call sites (AbstractJdbcCatalog, OceanBaseMySqlCatalog, YashanDbCatalog, JdbcCatalogUtils).

2.2 Test Coverage and Test Stability

New/updated unit tests directly targeting this round's fixes:

• CatalogUtilsTest: testSinglePhysicalTableResolvedFromAllResultColumns, testJoinIsNotDetectedAsSinglePhysicalTable, testExpressionColumnKeepsSingleTableUnverified, testSinglePhysicalTablePathUnverifiedOnMetadataFailure — cover the positive case, the join-rejection case, the newly-documented expression-column case, and the metadata-failure case.
• JdbcCatalogUtilsTest: 8 new/rewritten tests covering the verified-origin-path-is-used behavior (directly targeting @SEZ9 Issue 3), the multi-table-skip case, the missing-database-and-schema skip case, the table-not-found/no-columns/load-failure fallback cases, and 3 tests for withDefaultDatabase (targeting @SEZ9 Issue 4).
• E2E: JdbcMysqlTableOptionsIT.assertSinkTableOptions gained new assertions on the sink's SHOW CREATE TABLE output (column comments, table comment) against a source table configured in query-only mode via the pre-existing jdbc_mysql_sink_with_table_options.conf. This is a genuine end-to-end exercise of the generic (AbstractJdbcCatalog.resolveQueryTable) code path through a real container, not just mocks — good addition, since it was previously only unit-tested.

All new/changed unit tests are deterministic pure-function tests using Mockito stubs, AtomicBoolean flags, and synchronous lambdas — no sleeps, no shared mutable state, no timing dependency. Test Stability rating: Stable (evidence: CatalogUtilsTest.java and JdbcCatalogUtilsTest.java new/updated tests use only mocked ResultSetMetaData/DatabaseMetaData and plain assertions).

2.3 Documentation Updates

docs/en/docs/zh for Jdbc.md were updated to accurately describe the expression-column behavior (see Issue 1 resolution above), and both incompatible-changes.md files gained a detailed, accurate new entry. This closes the documentation gaps from both my own previous review and @SEZ9's Issues 5/6. I did not find any remaining inconsistency between the doc wording and the actual code behavior in this round.

3. Architectural Soundness

3.1 Elegance of the Solution

Good structural improvement over the previous head: collapsing "get the query table" and "verify single-table-ness" into one resolveQueryTable call per dialect is a cleaner shape than the previous two-call design, and it's what naturally forced the double-execution/connection-handling bugs to be visible and fixed together, exactly as I predicted in my 2026-08-29 comment ("the connection-handling fix may touch the same lines as the double-execution fix").

3.2 Maintainability

Good — QueryTableResolution is a small, self-explanatory value type; withDefaultDatabase and getSinglePhysicalTablePath are both small, pure, and unit-testable without a real JDBC connection.

3.3 Extensibility

The per-dialect resolveQueryTable override pattern is a strict improvement over the previous per-dialect isSinglePhysicalTableQuery override: a future dialect that needs a workaround now only has to implement one method that returns both pieces of information from one execution, rather than risk forgetting to reuse the metadata from a separately-overridden getTable.

3.4 Historical-Version Compatibility

Unchanged from both previous rounds' conclusion: row/column structure and order are preserved, so checkpoint/savepoint restore for already-running jobs is unaffected. The risk surface remains confined to newly-derived CatalogTable metadata at discovery/sink-auto-create time, and that upgrade-visible surface is now properly disclosed per Section 2.3.

4. Issue Summary

Number	Issue	Location	Severity
1	setMaxRows(1) caps the client-side ResultSet but may not prevent full server-side result transfer/buffering for MySQL-protocol drivers (OceanBase) in default non-streaming mode; worth confirming against the actual driver or hardening with a server-side LIMIT/streaming fetch	OceanBaseMySqlCatalog.java:236, YashanDbCatalog.java:208	Medium

All issues from both of my previous review rounds, and all eight issues from @SEZ9's review, are verified fixed in this head — see the itemized verification in Section 1.1.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed

   • None. The two items @SEZ9 and I jointly agreed were blockers (YashanDB connection-cache poisoning, unbounded double execution) are both verified fixed in this head.

2. Recommended fixes — non-blocking

   • Issue 1 (Medium): confirm whether setMaxRows(1) genuinely limits server-side result transfer for the OceanBase driver, or replace it with a server-side-limited probe query / streaming fetch for a real guarantee against large-table OOM at discovery time.

Overall assessment: this is a thorough, well-targeted follow-up commit that closes a real production-impacting bug (the YashanDB connection-cache poisoning would have broken all subsequent catalog operations on that connection, not just this feature) alongside seven other legitimate findings from @SEZ9's review and my own two prior findings, all backed by new, deterministic, well-scoped unit tests plus a genuine E2E assertion improvement. I don't see anything here that should block merge. As I noted in my previous rounds, I have read-only review rights on this repository (Comment only, not Approve/Request-changes) — a write-capable maintainer should do the final approval and merge, and @SEZ9 may want to re-confirm and clear their CHANGES_REQUESTED review now that all eight of their findings are addressed.

Thanks again for the fast, thorough turnaround on a genuinely tricky connection-handling bug, @yzeng1618 — and thank you @SEZ9 for the sharper second pass that caught what both of my rounds missed.

[DanielLeens]

Sorry for missing this in my last review round — I want to open with that before anything else. When I reviewed head 227f4bf0033e and concluded "Ready to merge after fixes", the fork's own CI Build run for that exact commit was already red (https://github.com/yzeng1618/seatunnel-dev/actions/runs/33361852878), and the failure was a direct, concrete manifestation of the very compatibility risk I had just finished documenting as "disclosed but intentional": JdbcDmSaveModeIT failed with dm.jdbc.driver.DMException: Data type mismatch because the default (then ALL-equivalent) merge injected the underlying table's primary key into a query-only source, which flipped the generated save-mode statement from INSERT to a Dameng MERGE INTO that Dameng can't prepare over LOB-ish columns. I checked the docs/incompatible-changes wording carefully but never actually looked at whether CI was green at that commit — that's on me, and it's exactly the kind of thing Step 0/CI-state-check in my own review method exists to catch. Filing this explicitly as a carryover, not a new-version-only issue.

The good news: this new commit (6bafd798fc59) is a well-targeted fix for precisely that regression. I re-traced the whole change from scratch against the current head rather than only diffing the delta.

What Problem Does This PR Solve?

• User pain point (this round, on top of the original #11969 fix): the query-only metadata merge landed in the previous commits unconditionally carried over the primary key, constraint keys and partition keys from the resolved underlying table. That silently switched any downstream sink using generate_sink_sql from plain INSERT to upsert/MERGE INTO semantics, which is exactly the risk both @SEZ9 and I flagged in earlier rounds as "upgrade-visible but disclosed" — and it turned out to actively break an existing E2E test (JdbcDmSaveModeIT) on Dameng because the generated MERGE INTO can't handle LOB-ish column comparisons.
• Fix approach: introduce a new source option query_table_metadata_merge (global or per table_list entry) with three modes — COMMENT (new default: merge only column/table comments and table options, leave identity/keys/partitions exactly as the query derived them), ALL (previous behavior — full merge, opt-in), NONE (skip resolving the underlying table entirely). The default is now the safe, non-behavior-changing choice.
• One-sentence summary: this commit correctly demotes the primary-key/constraint-key/partition-key merge from "always on" to "opt-in via ALL", which both fixes the concrete Dameng CI failure and closes the last open compatibility gap from my previous round — comments/options still flow through by default (the original #11969 pain point), but nothing that changes runtime semantics does.

Runtime flow reached by this change

Job planning time (source table discovery), catalog-backed path:
  JdbcCatalogUtils.getCatalogTable(tableConfig, jdbcCatalog)
    -> metadataMergeMode(tableConfig)                                   [NEW] resolves per-table
         override, else JdbcSourceOptions.QUERY_TABLE_METADATA_MERGE default (COMMENT)
    -> if NONE: jdbcCatalog.getTable(query) and return directly          [NEW] short-circuit, no
         resolveQueryTable() call at all — cheapest path
    -> else: jdbcCatalog.resolveQueryTable(query)                        (unchanged from last round)
    -> mergeWithUnderlyingTable(tableOfQuery, singlePhysicalTablePath, loader, metadataMergeMode)
         -> if NONE (defensive re-check): return tableOfQuery
         -> if ALL: mergeCatalogTable(tableOfPath, tableOfQuery)         (unchanged full merge)
         -> if COMMENT: mergeCommentsAndOptions(tableOfPath, tableOfQuery) [NEW] keeps
              tableOfQuery's TableIdentifier, PrimaryKey, ConstraintKeys, PartitionKeys;
              only replaces column comments (by name+SqlType match), table comment, options

No-catalog fallback path (JdbcCatalogUtils.java:507-520): symmetric treatment — same
metadataMergeMode() lookup, same NONE short-circuit, same mode threaded into
mergeWithUnderlyingTable.

Neither branch is reachable from the reader/writer task lifecycle or from
checkpoint/savepoint state — this is exclusively source-table-discovery/planning-time code,
same conclusion as my last two rounds.

1. Code Change Review

1.1 Core Logic Analysis

JdbcCatalogUtils.java:277-296 (catalog path) and :505-520 (no-catalog path) both now compute metadataMergeMode(tableConfig) before touching the underlying table, and short-circuit to jdbcCatalog.getTable(query) / the plain getCatalogTable(resultSetMetaData, ...) when the mode is NONE — this avoids even calling resolveQueryTable/getSinglePhysicalTablePath, so NONE genuinely restores pre-#11971 behavior with zero extra metadata probing, not just a no-op merge.

The new mergeCommentsAndOptions(CatalogTable tableOfPath, CatalogTable tableOfQuery) (JdbcCatalogUtils.java:588-627) is a clean, minimal extraction: it reuses the exact same mergeColumnComments helper that mergeCatalogTable uses (correctly refactored out rather than duplicated — good catch keeping that DRY), and builds a fresh CatalogTable from tableOfQuery.getTableId() + tableOfQuery's own primary key/constraint keys/partition keys, only swapping in tableOfPath.getOptions() and tableOfPath.getComment(). I verified TableSchema.Builder.primaryKey(null) and .constraintKey(emptyList) are both safe no-ops (seatunnel-api/.../TableSchema.java:65-76), so a query-derived table with no PK/constraints round-trips correctly.

One thing I specifically checked because it's easy to get backwards: mergeCommentsAndOptions still takes the verified origin TablePath for tableOfPath (via the same singlePhysicalTablePath threaded from CatalogUtils.getSinglePhysicalTablePath/resolveQueryTable) that my previous round confirmed fixes @SEZ9's Issue 3 — that fix is untouched and still correctly wired into both merge modes.

1.2 Compatibility Impact

Fully compatible by default, and this is a genuine improvement over the previous head: row shape, column order, primary key, constraint keys, partition keys, sink insert/upsert semantics and split planning are now byte-for-byte unchanged from pre-#11971 behavior for every existing job that doesn't opt in. The only default-visible difference remains what #11969 originally asked for — comments and table options flow into auto-create sink DDL. Opting into ALL restores the previous (this-PR's-earlier-round) behavior for users who want it, with the tradeoff clearly documented. incompatible-changes.md (en + zh) was updated accurately and matches the code exactly — I compared the "Impact" prose against the actual default branch (COMMENT) line by line.

1.3 Performance / Side-Effect Analysis

Strict improvement: the NONE mode now skips the extra resolveQueryTable/metadata-probe work entirely (previously there was no way to opt out of the probe at all), and COMMENT mode does the same amount of work as before (one metadata fetch, no extra query execution) — this commit only changes what's done with the already-fetched metadata, not how much is fetched.

1.4 Error Handling and Logging

Unchanged from last round (still warn-level on the verification/load failure paths); the success log now additionally states the mode ("Merged the metadata (mode: {})...", JdbcCatalogUtils.java:576), which is a nice diagnostic addition — if a user reports "I set ALL but I'm not seeing the PK", the log line now confirms which mode actually ran.

2. Code Quality Assessment

2.1 Coding Standards

New QueryTableMetadataMergeMode enum has a proper ASF license header, class-level and per-constant Javadoc explaining the runtime-behavior implications of each mode (not just "what" but "why it matters" — exactly the standard this repo's CLAUDE.md asks for), and a @JsonCreator fromString for case-insensitive table_list parsing consistent with how other enum-typed options in this connector are parsed elsewhere. mergeColumnComments extraction removes duplication instead of copy-pasting the column-comment-merge logic into the new method — good judgment call.

2.2 Test Coverage and Test Stability

New/updated tests directly targeting this round:

• JdbcSourceTableConfigTest (new file): default-is-COMMENT, global-option-propagation with lowercase parsing, and per-table override-beats-global — covers the three propagation cases that matter.
• JdbcCatalogUtilsTest: testMergeWithUnderlyingTableDefaultModeMergesCommentsAndOptionsOnly, testMergeWithUnderlyingTableAllModeMergesKeys (renamed from the old single test), testMergeWithUnderlyingTableNoneModeSkipsMerge (asserts the loader is never invoked — correctly verifies the short-circuit, not just the output), and testMergeCommentsAndOptionsSkipsRenamedOrTransformedColumns (covers a renamed column and a SqlType-mismatched column in the same test, both correctly falling back to the query-derived definition).
• All existing tests were updated to pass the new metadataMergeMode parameter rather than being weakened or dropped — I diffed every changed assertion against the previous round and confirmed no existing assertion was loosened.

All tests are deterministic (Mockito-free plain object construction + AtomicBoolean invocation flags, no sleeps, no shared state). Test Stability rating: Stable.

One gap: no test directly exercises metadataMergeMode()'s null-coalescing helper (JdbcCatalogUtils.java:298-302) in isolation, though it's implicitly covered end-to-end by JdbcSourceTableConfigTest's default-is-COMMENT case combined with the merge tests. Not blocking.

2.3 Documentation Updates

docs/en and docs/zh for Jdbc.md (prose paragraph + new options-table row) and incompatible-changes.md were all updated in this commit and are consistent with each other and with the code — I compared the English and Chinese incompatible-changes.md entries side by side and they say the same thing.

3. Architectural Soundness

3.1 Elegance of the Solution

This is the right shape for the fix: rather than trying to special-case Dameng (or picking a different default merge algorithm), it exposes the actual tradeoff (metadata-only vs. behavior-changing) as an explicit, documented, per-table-overridable option and picks the conservative default. That's more maintainable than a driver-specific workaround and gives every user — not just Dameng users — control over the tradeoff.

3.2 Maintainability

Good — QueryTableMetadataMergeMode is a small, self-documenting enum; mergeCommentsAndOptions is a pure function parallel in shape to the existing mergeCatalogTable, so a future reader can compare the two side by side to understand exactly what each mode does or doesn't touch.

3.3 Extensibility

The NONE/COMMENT/ALL three-tier design leaves room for a future finer-grained mode (e.g. "comments + partition keys but not PK") without a breaking change to the option's type, since it's a plain enum with room to add values.

3.4 Historical-Version Compatibility

This is the headline improvement of this round: historical/upgrading jobs are now provably unaffected by default (see 1.2), closing the exact gap that caused the Dameng CI regression at the previous head.

4. Issue Summary

No blocking issues found in this round's diff. Residual risk carried over from my last round, still open and still non-blocking:

Issue 1 — Location: seatunnel-connectors-v2/connector-jdbc/.../catalog/oceanbase/OceanBaseMySqlCatalog.java:236, .../catalog/yashandb/YashanDbCatalog.java:208

• Problem description: Statement.setMaxRows(1) caps the client-side ResultSet size but, per documented MySQL-protocol JDBC driver behavior in the default non-streaming fetch mode, does not necessarily limit what's transferred/buffered server-side before truncation.
• Potential risk: a query-only OceanBase/YashanDB source over an unbounded query (SELECT * FROM huge_table) could still buffer the full result client-side at planning time.
• Best improvement: confirm against the actual OceanBase/YashanDB driver, or harden with a server-side LIMIT-wrapped probe / explicit streaming fetch size.
• Severity: Medium.
• Raised by another reviewer: No (carried over from my own previous round).

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed

   • None.

2. Recommended fixes — non-blocking

   • Issue 1 (Medium, carried over): confirm setMaxRows(1) behavior on the actual OceanBase/YashanDB driver or replace with a server-side-limited probe.

Overall assessment: this commit fixes a real, CI-observed regression (JdbcDmSaveModeIT on Dameng) that traces directly back to a risk both @SEZ9 and I had already identified but under-weighted as "disclosed, not blocking" in the previous round. Making the safe (COMMENT) mode the default while keeping the full merge available via ALL is the right fix, it's backed by solid new unit tests with no weakened assertions anywhere in the diff, and both language docs are accurate and consistent. Again, apologies for not catching the red CI at the previous head myself — thank you for turning this around quickly, @yzeng1618. As always, I have comment-only rights on this repo, so a write-capable maintainer should do the final CI check (the fork's Build run for this head was still queued as of this review) and merge.

[DanielLeens]

Quick follow-up: at the time of my last review the fork's Build run was still queued, so I couldn't confirm a green result. It has since completed successfully on this same head with no new commit in between.

Combined with the "no blockers" conclusion from my last review, this is clear from my side. A write-capable maintainer will still need to submit the actual approval, since I only have comment rights here. Thanks for turning the fix around quickly!

[DanielLeens]

CI was the only remaining item in my previous review — the fork's Build run for this head (6bafd798fc59) was still queued at that time, and there were no outstanding code blockers (Issue 1, the setMaxRows(1) driver-behavior question, was already carried over as non-blocking). Build is now green, so I'm updating my conclusion to approve.

Conclusion: Ready to merge

[davidzollo]

Please solve conflicts~

[DanielLeens]

What Problem Does This PR Solve?

A JDBC source defined purely with query (no table_path) derives its schema only from ResultSetMetaData, which carries no column/table comments, primary/constraint keys, or partition keys — sinks that auto-create tables (Doris, StarRocks, MySQL, ...) lose that metadata. This PR resolves the underlying physical table when JDBC metadata verifies every selected column originates from it, and merges that table's metadata into the query-derived schema.

Note on this re-review round

This PR is labeled NEED_REREVIEW because the head SHA changed after my last APPROVED review (6bafd798fc). I traced the actual history first: the new head (bbb76bd99a) is a Merge branch 'dev' into dev-query-only commit made today in direct response to @davidzollo's comment this morning ("Please solve conflicts~"). I diffed the merge commit against the pre-merge head restricted to this PR's own files (connector-jdbc, Jdbc.md, incompatible-changes.md) and confirmed zero of this PR's files changed in the merge — the merge only pulled in unrelated dev changes (hundreds of files across other connectors/docs/benchmarks) and resolved the conflict davidzollo flagged. mergeStateStatus/mergeable now report MERGEABLE (no conflicts), confirming the resolution succeeded. So there is no new functional diff to review this round for this PR's own scope, but per the task I still performed a complete, independent fresh trace of the full current diff and runtime path, re-verifying the load-bearing claims from my last review against the current source rather than assuming they still hold.

1. Code Change Review

1.1 Core Logic Analysis

This PR went through a genuinely productive multi-round exchange with @SEZ9, who found 8 issues (2 High: a YashanDB connection-cache-poisoning bug and an unbounded double query execution; 6 Medium) on an earlier head. I independently re-verified, against the current source (not carried over from memory), that all 8 are fixed:

• YashanDB connection handling — YashanDbCatalog.resolveQueryTable (YashanDbCatalog.java:205-208) keeps Connection defaultConnection = getConnection(defaultUrl) outside try-with-resources, with only the Statement inside it, and carries a Javadoc warning that the connection "is cached and shared, so it must not be closed here." Confirmed present in the current worktree.
• Bounded/single execution — both OceanBaseMySqlCatalog.resolveQueryTable (OceanBaseMySqlCatalog.java:226-236) and YashanDbCatalog.resolveQueryTable call setMaxRows(1) before executeQuery, and the query is now executed once (not twice) per table via the unified resolveQueryTable shape.
• Verified-origin-path threading — mergeWithUnderlyingTable takes the verified Optional<TablePath> directly as a parameter instead of re-deriving a possibly-different path from the query-derived TableIdentifier.
• Null-database handling — JdbcCatalogUtils.withDefaultDatabase(TablePath, String) (JdbcCatalogUtils.java:526) is applied before the tableExists/getTable lookup, confirmed present at JdbcCatalogUtils.java:288.
• Documentation of the upgrade-visible behavior change — docs/en/introduction/concepts/incompatible-changes.md:20-24 (and the zh counterpart) has a dedicated "JDBC Connector" entry titled "Behavior change: query-only JDBC sources merge the underlying table's comments and options," confirmed present in the current worktree.
• Fallback logging raised from debug to warn — confirmed in JdbcCatalogUtils.java.

I did not find anything in this pass that either reviewer's previous rounds missed — this is a case where the iterative review process (my two rounds + @SEZ9's sharper second pass + my final confirmation) already did its job before this re-review was even triggered.

1.2 Compatibility Impact

Fully compatible for already-running jobs' checkpoint/restore and row semantics. Row shape and column order are unaffected in every branch. There is a disclosed, intentional behavior change for newly-planned query-only jobs (auto-create sinks may now emit a PK/comments/options they didn't have before), which is exactly the point of the PR and is now properly recorded in incompatible-changes.md as required for a behavior-changing PR.

1.3 Performance / Side-Effect Analysis

Substantially improved from earlier heads: OceanBase/YashanDB now execute the discovery query once (capped via setMaxRows(1)) instead of twice, unbounded. One residual, already-flagged caveat remains open as a non-blocking follow-up (Issue 1 below) — carried over from my own previous review, not new.

1.4 Error Handling and Logging

All fallback paths (resolveQueryTable's catch, getSinglePhysicalTablePath's catch, mergeWithUnderlyingTable's loader-failure catch) log at warn with the table path and exception, addressing @SEZ9's silent-fallback finding. Confirmed unchanged and still present on the current head.

Issue 1 (Medium, carryover from my previous review, not new this round) — setMaxRows(1) caps the client-side ResultSet a JDBC driver returns, but for MySQL-protocol drivers running in default (non-streaming, fetchSize=0) mode, the documented behavior of several driver families is that the entire result set is still fetched from the server into client memory before being truncated to the max-rows limit. OceanBase's driver is MySQL-protocol-compatible, so this caveat plausibly still applies there.

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oceanbase/OceanBaseMySqlCatalog.java:236, seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/yashandb/YashanDbCatalog.java:208
• Potential risk: a query-only OceanBase source over an unbounded query (e.g. SELECT * FROM huge_table) could still transfer/buffer the full result set client-side at job-planning time even with the cap set — a narrower version of the original OOM concern (one execution instead of two, but still potentially unbounded transfer).
• Best improvement: if confirmed against the actual OceanBase driver, wrap the probe as a server-side-limited query (SELECT * FROM (<query>) t LIMIT 1) or enable streaming fetch (setFetchSize(Integer.MIN_VALUE)) before executing, instead of relying solely on client-side setMaxRows.
• Severity: Medium (a genuine improvement already landed here; this is about closing the remaining gap, not a regression).
• Raised by another reviewer: No (I raised this in my previous review round; still open as a disclosed non-blocking follow-up, not fixed in this head since this head carries no new content for this PR's files).

2. Code Quality Assessment

2.1 Coding Standards

Unchanged from my last review: AbstractJdbcCatalog.resolveQueryTable, QueryTableResolution, CatalogUtils.getSinglePhysicalTablePath, and JdbcCatalogUtils.withDefaultDatabase all carry Javadoc, including the non-obvious "must not be closed here" constraint on the YashanDB shared connection. No stale references to the old isSinglePhysicalTable/isSinglePhysicalTableQuery names remain anywhere in connector-jdbc.

2.2 Test Coverage and Test Stability

Unchanged (no new commits touched tests or source this round — only the no-op dev merge). Coverage remains thorough: CatalogUtilsTest (single-table resolution, join-rejection, expression-column-unverifiable, metadata-failure), JdbcCatalogUtilsTest (verified-origin-path usage, multi-table skip, missing-database-and-schema skip, withDefaultDatabase x3), and a genuine end-to-end JdbcMysqlTableOptionsIT.assertSinkTableOptions assertion against a real container's SHOW CREATE TABLE output.
Stability rating: Stable — all unit tests use mocked ResultSetMetaData/DatabaseMetaData with plain synchronous assertions, no sleeps or shared mutable state; the IT is a real-container assertion with no timing dependency in what it checks.

2.3 Documentation Updates

docs/en/docs/zh Jdbc.md and both incompatible-changes.md files are consistent with the current code and with each other; I found no leftover inconsistency in this pass.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. Collapsing "get the query table" and "verify single-table-ness" into one resolveQueryTable call per dialect is a clean structural improvement that made the double-execution and connection-handling bugs visible and fixable together.

3.2 Maintainability

Good — QueryTableResolution is a small, self-explanatory value type; withDefaultDatabase and getSinglePhysicalTablePath are small, pure, and unit-testable without a real JDBC connection.

3.3 Extensibility

The per-dialect resolveQueryTable override pattern is a strict improvement over the previous per-dialect isSinglePhysicalTableQuery hook for any future dialect needing a workaround.

3.4 Historical-Version Compatibility

Row/column structure and order are preserved, so checkpoint/savepoint restore for already-running jobs is unaffected. The upgrade-visible surface (query-only sources gaining PK/comments/options) is properly disclosed per Section 2.3.

4. Issue Summary

#	Issue	Location	Severity
1	setMaxRows(1) may not prevent full server-side result transfer/buffering for MySQL-protocol drivers (OceanBase) in default non-streaming mode; unconfirmed against the actual driver (carryover, still open)	OceanBaseMySqlCatalog.java:236, YashanDbCatalog.java:208	Medium

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed
   • None. Both items @SEZ9 and I jointly flagged as blockers (YashanDB connection-cache poisoning, unbounded double execution) were verified fixed in an earlier round and remain fixed on this head (no regression from today's dev merge).
2. Recommended fixes — non-blocking
   • Issue 1 above: confirm whether setMaxRows(1) genuinely limits server-side transfer for the OceanBase driver, or harden with a server-side-limited probe query / streaming fetch.

Merge-conflict status: @davidzollo flagged merge conflicts this morning; the author's dev merge (today's head, bbb76bd99a) resolved them — mergeStateStatus/mergeable now report MERGEABLE. CI status: the Build check for this exact merged head was still queued at review time; since the merge touched no files belonging to this PR's own scope, I don't expect a functional regression, but please confirm it actually goes green before merging rather than relying on this assessment alone.

Overall: this is a thorough, well-tested fix that survived a genuinely sharp second-reviewer pass (thank you @SEZ9) and closed every issue raised, including a real production-impacting connection-cache-poisoning bug. I have read-only review rights here, so a write-capable maintainer still needs to give the final approval once the merge-resolution CI run is confirmed green.

[DanielLeens]

Quick follow-up: I checked the current head (bbb76bd99a) again and the Build check that was still queued at my last review time has now completed successfully, with no new commits since. That confirms the merge-conflict resolution didn't introduce any regression, and there is no remaining blocker from my side.

Conclusion: Ready to merge

No blockers. I have read-only review rights on this repository, so a write-capable maintainer still needs to give the final approval before this is merged.

Thanks again for the thorough work on this one, @det101 — and thanks to @SEZ9 for the sharp second-reviewer pass.

[SEZ9]

@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.

[DanielLeens]

@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.

[davidzollo]

+1
LGTM

Thanks for your contribution! This is a solid addition to the project, and your attention to detail really shows. Contributors like you are what keep this community moving forward. Feel free to keep the PRs coming — we're always glad to have you here!

Please ignore the following message if you have joined in the Slack~

To facilitate focused communication among contributors, we’ve created a brand-new dedicated #contributors channel in official Slack workspace.
Slack workspace link: https://s.apache.org/seatunnel-slack

This channel is designed for technical discussions, feature design brainstorming, roadmap sync and direct communication with project core Contributors. Everyone who contributes code, docs, test cases or community ideas is warmly invited to join, share thoughts and collaborate together under The Apache Way. Welcome to join 😄

[SEZ9]

Thanks for keeping the branch in sync. Since the change between 6907e2f2f8fb and e1d06f9021c7 looks like a merge from dev only, I'd like to confirm the status of the earlier points rather than assume they're closed:

• PR11971-F1 – Can you confirm YashanDbCatalog no longer closes the shared cached connection when checking for a single-physical-table query?
• PR11971-F2 / PR11971-F7 – Do the OceanBase/YashanDB overrides now bound the probe query (e.g. a row limit) and reuse a single ResultSetMetaData, and does the default catalog path avoid executing the user query twice?
• PR11971-F3 – Does mergeWithUnderlyingTable now load the metadata-verified origin TablePath rather than the query-derived identifier?
• PR11971-F4 – For drivers that report only a schema and no catalog, does the resolved TablePath still end up with a null database name? If so, tableExists/getTable would throw and the merge would silently fall back. Could you either handle the schema-only case explicitly or point me to where it is covered?
• PR11971-F5 – Is there a changelog entry describing query_table_metadata_merge and its COMMENT default? If not, please add one.
• PR11971-F6 – With ALL, query-only pipelines gain a merged primary key on upgrade, which affects split planning and sink auto-create DDL. Are the incompatible-changes notes updated for this? Alternatively, if COMMENT is intentionally the only behavior applied without opt-in, please state that explicitly in the docs.
• PR11971-F8 – Are fallback failures now logged at a visible level instead of being swallowed at DEBUG?

A quick pointer to the relevant lines for each would be enough. Once those are confirmed I'm happy to do a final pass.

[yzeng1618]

Thanks for keeping the branch in sync. Since the change between 6907e2f2f8fb and e1d06f9021c7 looks like a merge from dev only, I'd like to confirm the status of the earlier points rather than assume they're closed:

• PR11971-F1 – Can you confirm YashanDbCatalog no longer closes the shared cached connection when checking for a single-physical-table query?
• PR11971-F2 / PR11971-F7 – Do the OceanBase/YashanDB overrides now bound the probe query (e.g. a row limit) and reuse a single ResultSetMetaData, and does the default catalog path avoid executing the user query twice?
• PR11971-F3 – Does mergeWithUnderlyingTable now load the metadata-verified origin TablePath rather than the query-derived identifier?
• PR11971-F4 – For drivers that report only a schema and no catalog, does the resolved TablePath still end up with a null database name? If so, tableExists/getTable would throw and the merge would silently fall back. Could you either handle the schema-only case explicitly or point me to where it is covered?
• PR11971-F5 – Is there a changelog entry describing query_table_metadata_merge and its COMMENT default? If not, please add one.
• PR11971-F6 – With ALL, query-only pipelines gain a merged primary key on upgrade, which affects split planning and sink auto-create DDL. Are the incompatible-changes notes updated for this? Alternatively, if COMMENT is intentionally the only behavior applied without opt-in, please state that explicitly in the docs.
• PR11971-F8 – Are fallback failures now logged at a visible level instead of being swallowed at DEBUG?

A quick pointer to the relevant lines for each would be enough. Once those are confirmed I'm happy to do a final pass.

Verified at e1d06f9021c7:

• PR11971-F1 — Resolved. YashanDbCatalog.resolveQueryTable closes only the Statement and ResultSet; the shared cached Connection remains open. See YashanDbCatalog.java#L199-L241.

• PR11971-F2 / PR11971-F7 — Resolved. Both dialect overrides use setMaxRows(1) and a single ResultSetMetaData for origin verification. The default AbstractJdbcCatalog path uses PreparedStatement.getMetaData(), so the user query is not executed a second time. See OceanBaseMySqlCatalog.java#L222-L282, YashanDbCatalog.java#L199-L241, and AbstractJdbcCatalog.java#L700-L718.

• PR11971-F3 — Resolved. mergeWithUnderlyingTable receives the metadata-verified singlePhysicalTablePath separately from the query-derived table and uses the verified path when loading the underlying table. The regression test also uses different query and origin paths. See JdbcCatalogUtils.java#L549-L583 and JdbcCatalogUtilsTest.java#L674-L692.

• PR11971-F4 — The catalog-backed path handles schema-only metadata through withDefaultDatabase, filling the database from the configured default before calling tableExists/getTable. The direct JDBC metadata fallback does not call tableExists/getTable; it loads metadata through CatalogUtils.getCatalogTable. Tests cover missing database, existing database, and the no-default-database case. See JdbcCatalogUtils.java#L282-L300, JdbcCatalogUtils.java#L500-L536, and JdbcCatalogUtilsTest.java#L785-L808.

• PR11971-F5 — Yes. query_table_metadata_merge and its COMMENT default are documented in both English and Chinese JDBC documentation. The behavior is also documented in both incompatible-changes documents. See Jdbc.md#L50, Jdbc.md#L166, and incompatible-changes.md#L51-L67.

• PR11971-F6 — The default COMMENT mode only merges comments. Primary keys, constraints, partition keys, and table options remain unchanged by default. ALL is an explicit opt-in and its impact on sink DDL, insert/upsert behavior, and split planning is documented.

• PR11971-F8 — Metadata verification failures and underlying-table loading failures are logged at WARN with the table path and exception before falling back to the query-derived table. See AbstractJdbcCatalog.java#L710-L718 and JdbcCatalogUtils.java#L575-L583. The remaining DEBUG log in YashanDB is only for optional primary-key enrichment.

All eight points are covered at the current head.

[DanielLeens]

Thanks for keeping this branch in sync again, @yzeng1618! I re-reviewed the current head (4d4a6ecfca20) from scratch. The only change since my last round (e1d06f9021c7) is another Merge branch 'dev' commit. I compared the diff of this PR's own 17 files (connector-jdbc, the EN/ZH docs, incompatible-changes.md and JdbcMysqlTableOptionsIT) against the merge-base of dev at both heads, and the resulting patch is byte-for-byte identical (same patch-id), so the code I reviewed last round is unchanged. All earlier findings from me, @SEZ9 and @davidzollo remain closed at this head. (This is a carryover of the last round's conclusion, nothing new was missed.)

What Problem Does This PR Solve?

• User pain point: a JDBC source configured with only query derives its schema from ResultSetMetaData, which has no column or table comments, keys or partition keys, so auto-creating sinks (Doris, StarRocks, MySQL, ...) lose them.
• Fix approach: when JDBC origin metadata proves every result column comes from one physical table, resolve that table and merge its metadata into the query-derived schema, controlled by the new query_table_metadata_merge option (COMMENT default / ALL / NONE). Any failure falls back to the query-derived table.
• Summary: query-only sources keep their comments without changing runtime behavior by default.

1. Code Change Review

1.1 Core Logic Analysis

JdbcCatalogUtils.getCatalogTable(tableConfig, jdbcCatalog)
  -> NONE ? jdbcCatalog.getTable(query)                       (old behavior)
  -> jdbcCatalog.resolveQueryTable(query)                      AbstractJdbcCatalog
       default: getTable(query) + PreparedStatement.getMetaData() probe
       OceanBaseMySql / YashanDb overrides: one Statement.executeQuery with setMaxRows(1),
         one ResultSetMetaData reused for schema + origin check, shared connection NOT closed
  -> CatalogUtils.getSinglePhysicalTablePath(metadata)         every column must share one catalog/schema/table
  -> mergeWithUnderlyingTable(...)
       COMMENT -> comments only | ALL -> existing mergeCatalogTable
       any exception -> log.warn + query-derived table

The normal user path (a source with query and no table_path) does reach this logic, and the direct-connection fallback path is covered the same way. Defaults are conservative: COMMENT keeps the query-derived identity, options, keys and partition keys, so sink DDL, upsert semantics and split planning are unchanged unless the user opts into ALL. Joins, expression columns and missing origin metadata keep the query-derived table untouched. No code in this PR changed since my last round.

1.2 Compatibility Impact

Fully compatible. The option and the merge mechanism are new in this unmerged PR, so no released behavior changes. NONE restores exactly the old behavior. The option is registered in JdbcSourceFactory.optionRule() and in JdbcSourceTableConfig for table_list entries, and the EN/ZH Jdbc.md and incompatible-changes.md entries are consistent with the code. No serialization, checkpoint or protocol format is touched.

1.3 Performance / Side-Effect Analysis

At most one extra metadata probe at planning time, never on the per-row path, so there is no memory, lock, retry or checkpoint impact. The OceanBase/YashanDB overrides execute the user query once with setMaxRows(1) and do not close the shared cached connection. The one residual, already disclosed earlier, is that setMaxRows(1) may not stop a driver from buffering server-side before returning metadata. It is a planning-time cost only and non-blocking.

1.4 Error Handling and Logging

Failures are logged at WARN with the table path and then fall back to the query-derived table. That is the intended graceful degradation for a best-effort enrichment, not a swallowed error. No sensitive data is logged.

No formal issues at this head.

2. Code Quality Assessment

• 2.1 Coding standards: fine. New classes and non-trivial methods have Javadoc that explains purpose and constraints.
• 2.2 Test coverage / stability: Stable. JdbcCatalogUtilsTest, CatalogUtilsTest and JdbcSourceTableConfigTest cover single-table, join, missing-origin, mode and fallback cases, and JdbcMysqlTableOptionsIT is extended. They are deterministic with no sleeps or time-dependent assertions. In the fork run for this head, every jdbc/connector-jdbc related job and JdbcMysqlTableOptionsIT passed (details in the CI section).
• 2.3 Docs: EN and ZH Jdbc.md and incompatible-changes.md are updated and consistent.

3. Architectural Soundness

• 3.1: Precise fix with a reasonable long-term shape: anything that can change behavior is opt-in, and the origin check guards against joins and expression columns.
• 3.2 / 3.3: Boundaries are explicit (catalog hook, utility merge, mode enum), and the resolveQueryTable hook gives other dialects a clean extension point.
• 3.4: Compatible with historical versions, no upgrade action needed.

4. Issue Summary

#	Issue	Location	Severity
-	No formal issues	-	-

5. Merge Recommendation

Conclusion: Ready to merge once CI is green

1. Blockers (code): none. This conclusion is unchanged from my last round, and I agree with the earlier approval by @davidzollo (dismissed only because of a later push).
2. Remaining gate: CI only. The fork run for this head (https://github.com/yzeng1618/seatunnel-dev/actions/runs/38208858568) completed with exactly one failed job, updated-modules-integration-test-part-4 (11, ubuntu-latest); the JDK 17 leg of the same job passed. The failure is ClickhouseIT.clickhouseWithCreateSchemaWhenNotExist (row counts such as expected 100 but was 301/401/501... and expected 200 but was 201), in a Clickhouse e2e module this PR does not touch. The counts accumulate across parameterized containers, which looks like a table-state/ordering problem in that test, and the sibling JDK 17 leg passing points the same way. I did not find an open fix on dev for it, and this is my inference from the log, not a proven root cause. Note that dev just received #12686, which added more ClickhouseIT cases, so it is worth a look from the Clickhouse owners if it recurs on other PRs.
   Next step: please rerun only the failed job (updated-modules-integration-test-part-4 (11)). The branch is currently 1 commit behind dev (GitHub reports mergeable_state: blocked, only waiting for required checks and approval, with no conflicts), so a sync is not needed just for this.

The apache-side Build check is still a pointer to the fork run and was in progress when I looked. I only have read-level review rights here, so a write-capable maintainer still needs to give the final approval once CI is green. Thanks again for the careful iteration on this one!

[DanielLeens]

Update on the CI fact from my previous review (head 4d4a6ecfca20): CI was the only remaining gate, and it has now changed.

• My last review cited a failed fork job, updated-modules-integration-test-part-4 (11, ubuntu-latest) (ClickhouseIT, a module this PR does not touch), and said the apache-side Build was still in progress.
• The failed job was rerun. The fork run for this head (yzeng1618/seatunnel-dev run 38208858568, attempt 2) is now completed / success, and the apache-side Build check is pass for the same head SHA.
• There is no new commit since my review, and the code-side conclusion is unchanged: no formal issues at this head, and no non-CI blockers.

Since CI was the only remaining blocker and it is now passing, I am approving. Thanks @yzeng1618 for the careful iterations. A write-capable maintainer still needs to give the final approval/merge.

[DanielLeens]

The CI fact cited in my previous review has changed: Build for the current unchanged head is now passing. CI was previously the only remaining blocker, and I do not see a new source-side blocker from Daniel's review. GitHub now reports this branch as having merge conflicts with dev. Please sync with the latest dev and resolve the conflicts first, then rerun the required checks. This merge-state blocker is separate from the now-green Build result. I have read-only review controls, so this COMMENT does not change GitHub's displayed review decision; a write-capable maintainer must perform any final approval or dismissal action.

[nzw921rx]

default is NONE?
Maintain the old behavior for unconfigured users.

[DanielLeens]

Thanks @nzw921rx, fair challenge, and let me be precise about what the default does today and correct my own wording.

What the default does (head 4d4a6ecfc): JdbcSourceOptions.QUERY_TABLE_METADATA_MERGE defaults to COMMENT (JdbcSourceOptions.java, the new option), and JdbcCatalogUtils.metadataMergeMode(...) falls back to that default when the entry has no explicit value (JdbcCatalogUtils.java:296-300). So an unconfigured query-only source does take the new path (:277-294): it probes the query's result metadata, resolves the single underlying table, and merges only column comments and the table comment (mergeComments, :594-623). It keeps the query-derived identity, options, primary key, constraint keys and partition keys, so split planning, insert/upsert choice and row shape do not change.

Where it is not identical to the old behavior for unconfigured users:

• Auto-creating sinks (MySQL, Doris, StarRocks, ...) will now create target tables with the source column and table comments on the first run after upgrade (already-created sink tables are untouched). That is the intended user benefit, but it is a visible difference.
• Planning does one extra metadata probe plus a tableExists/getTable per query-only table; any failure logs a WARN and falls back to the query-derived table (:549-590).
• In my last review I called the PR "fully compatible". The more accurate phrase is "documented, low-risk behavior change by default"; the PR documents it in the incompatible-changes.md entry (en and zh). Sorry for the loose wording.

Your options, from smallest to largest change: (a) keep COMMENT and rely on the documented note, or (b) change the default to NONE so unconfigured jobs are byte-for-byte unchanged and the feature becomes opt-in. If you want (b), it is small: the option default (JdbcSourceOptions), the enum/option Javadoc that says "This is the default", the default column and prose in en/zh Jdbc.md, the incompatible-changes.md entry (it would become "new opt-in option"), and the tests that assert the default mode. The metadataMergeMode(...) fallback already reads defaultValue(), so no logic change is needed. The trade-off is that users with query-only sources only get comments after setting query_table_metadata_merge = COMMENT. Since SeaTunnel's guidance is not to change defaults casually, I would support (b) if that is the project's rule; it is your call as maintainer and I will re-review whichever way @yzeng1618 goes.

Separate from this: GitHub currently reports this branch as conflicting with dev (mergeable_state: dirty), so it needs a sync before it can merge.

[nzw921rx]

What kind of behavior will be generated when configuring both TABLE_PATH and QUERY_TABLE_MTADATA_MERGE at the same time, and are they mutually exclusive?

[DanielLeens]

Good question. Short answer: they are not mutually exclusive in validation, but they do not combine; table_path wins and query_table_metadata_merge is not consulted.

Trace (catalog path, JdbcCatalogUtils.getCatalogTable(tableConfig, jdbcCatalog, jdbcDialect)):

• table_path and query both set (JdbcCatalogUtils.java:246-270): this is the pre-existing branch. It loads the table from table_path, runs the query for the query-derived table, and calls mergeCatalogTable(tableOfPath, tableOfQuery), i.e. the full merge (the same result that ALL produces). It never reads getQueryTableMetadataMerge().
• table_path only (:272-275): plain jdbcCatalog.getTable(tablePath); the mode is not consulted.
• query only (:277-294): the only branch that reads metadataMergeMode(tableConfig): NONE returns the old jdbcCatalog.getTable(query), otherwise resolveQueryTable plus mergeWithUnderlyingTable.
  The direct-connection fallback has the same shape (:474-499 for table_path + query, :508-518 for query only).

The factory does not declare any exclusivity between them either: JdbcSourceFactory.optionRule() lists TABLE_PATH, QUERY and QUERY_TABLE_METADATA_MERGE as plain optionals (the only exclusivity rule is the table_list validator), and the Jdbc.md docs say query can be combined with table_path.

Consequence worth deciding: a config with table_path + query + query_table_metadata_merge = NONE still gets the full merge, and the option is silently ignored. That is consistent with the option description ("derived from a query-only source table"), but it can surprise someone who sets NONE expecting the old behavior. Two small options: document explicitly in en/zh Jdbc.md that the option only applies when table_path is not set, or log a WARN (or reject) when it is explicitly set together with table_path. I would put this at Low severity and leave the choice to you and @yzeng1618; a one-line doc sentence plus a unit test pinning that table_path + query ignores the mode would be enough.

[SEZ9]

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!

[DanielLeens]

Thanks @SEZ9. I checked your F1-F8 summary against 4d4a6ecf, because these have changed from the earlier implementation:

• F1/F2: YashanDbCatalog.resolveQueryTable keeps the cached connection outside try-with-resources (206), caps returned rows with setMaxRows(1) (208), and closes the statement/result set only. OceanBaseMySqlCatalog does the same at 234-243. A returned-row cap does not promise that the database avoids all expensive query planning/scanning, but neither path now materializes an unbounded result just to consume metadata.
• F3/F4/F7: JdbcCatalogUtils.java:281-293 consumes one QueryTableResolution, loads its verified physical path, and fills a blank database through withDefaultDatabase (526-531). It does not discard that path and reconstruct it from the query-derived identifier. The existing unit cases around 668 and 787 pin verified-origin and default-database handling.
• F8: verification fallback WARN is at AbstractJdbcCatalog.java:710-711; load fallback WARN is at JdbcCatalogUtils.java:580. These are not DEBUG-only on this head.
• F5/F6: EN Jdbc.md:50/166 and incompatible-changes.md:150 onward document that COMMENT is the default, preserving primary keys/options/constraints/partition keys, while ALL is explicit opt-in. The ZH mirrors document the same. Therefore unconditional new primary-key inference is not the default behavior of the final implementation. The PR description still contains the older broad merge description and would benefit from being aligned with COMMENT versus ALL; that is not evidence the old implementation is still running.

My prior note about table_path plus query taking precedence over the query-only mode remains a non-blocking scope clarification. This reply does not close your review on your behalf or constitute another full code review. Build is green, but GitHub now reports conflicts against dev. Please resolve those conflicts and rerun checks on the new head; the full updated diff and final write-capable maintainer review will then need a fresh pass.

Current status evidence: head 4d4a6ecfca200e39e2d489d1b233bba2948f027d, base dev, compare diverged (ahead 12, behind 87), mergeable state dirty.

[nzw921rx]

Request changes: query-only metadata merge only works for MySQL in real-database tests

Thanks for the iterations; the COMMENT default and the defensive fallback are the right direction. I tested the current head (4d4a6ecf) against real databases because the change is generic JDBC code and the only new real-DB test is MySQL. The results show the feature does not work for several supported databases, and ALL has a join-related correctness risk.

How I tested

Environment: Docker Desktop 28.5.1 (Linux aarch64, 8 CPU / 12 GB), JDK 17.0.20.1, Maven wrapper offline.

Containers / databases:

• MySQL 8.0.43
• PostgreSQL 14-alpine
• Oracle Free 23-slim
• SQL Server 2019
• DuckDB 1.3.1 (embedded)

For each database I created a table with a table comment, column comments and a primary key. Examples of the DDL:

-- MySQL
CREATE TABLE meta.t (
  id BIGINT NOT NULL COMMENT 'id comment',
  name VARCHAR(64) COMMENT 'name comment',
  PRIMARY KEY (id)
) COMMENT='table comment';

-- PostgreSQL / Oracle / DuckDB
CREATE TABLE public.t (id BIGINT PRIMARY KEY, name VARCHAR(64));
COMMENT ON TABLE public.t IS 'table comment';
COMMENT ON COLUMN public.t.id IS 'id comment';
COMMENT ON COLUMN public.t.name IS 'name comment';

-- SQL Server
CREATE TABLE dbo.t (id INT NOT NULL PRIMARY KEY, name NVARCHAR(64));
EXEC sp_addextendedproperty 'MS_Description','table comment','SCHEMA','dbo','TABLE','t';
EXEC sp_addextendedproperty 'MS_Description','id comment','SCHEMA','dbo','TABLE','t','COLUMN','id';
EXEC sp_addextendedproperty 'MS_Description','name comment','SCHEMA','dbo','TABLE','t','COLUMN','name';

The harness uses the public path, with real drivers and real catalogs (no mocked ResultSetMetaData):

JdbcConnectionConfig connection = JdbcConnectionConfig.builder()
    .url(url).driverName(driver).username(user).password(password).build();

JdbcSourceTableConfig table = JdbcSourceTableConfig.builder()
    .query("SELECT * FROM " + tableName)
    .queryTableMetadataMerge(QueryTableMetadataMergeMode.COMMENT)
    .build();

Map<TablePath, JdbcSourceTable> tables =
    JdbcCatalogUtils.getTables(connection, List.of(table), null);
CatalogTable catalogTable = tables.values().iterator().next().getCatalogTable();

Command:

./mvnw -o -pl :connector-jdbc -Dskip.spotless=true -Dlicense.skipAddThirdParty=true \
  -Dtest=TmpQueryOnlyMetadataTest test

I also printed resolveQueryTable(...).getSinglePhysicalTablePath() to distinguish "path not resolved" from "path resolved but not merged", and, for Oracle/SQL Server/DuckDB, executed the query and re-read ResultSetMetaData to rule out lazy metadata.

Results: query-only + COMMENT

Database	getSinglePhysicalTablePath()	Table comment	Column comments	Verdict
MySQL 8.0.43	meta.t	merged	merged	PASS
PostgreSQL 14	t	not merged	not merged	FAIL
Oracle Free 23	empty	not merged	not merged	FAIL
SQL Server 2019	empty	not merged	not merged	FAIL
DuckDB 1.3.1	empty	not merged	not merged	FAIL

For PostgreSQL the metadata reports table=t but no catalog/schema. mergeWithUnderlyingTable returns early when both database and schema are blank (JdbcCatalogUtils.java:557-564), so the underlying table is never loaded.

For Oracle, SQL Server and DuckDB, ResultSetMetaData.getTableName() is empty. Executing the query and re-reading ResultSetMetaData produced the same empty table/catalog/schema, so this is driver behavior, not a lazy-metadata artifact. CatalogUtils.getPhysicalTablePath returns empty (CatalogUtils.java:345-382) and no merge is attempted.

Other behaviors

Behavior	MySQL	Other tested DBs
query-only + ALL	PASS (comments, PK, options)	PostgreSQL/Oracle/SQL Server/DuckDB still do not merge
query-only + NONE	old behavior; no comment merge	not applicable (no merge either way)
table_path + query	PASS	PASS for PostgreSQL, Oracle, SQL Server, DuckDB (column comments; SQL Server/DuckDB catalogs do not expose the table comment)
Join of two tables, selecting both sides	correctly skipped	-
Join of one side (SELECT t.* FROM t JOIN t2 ...) + ALL	incorrectly treated as single-table; t's PK/options merged	-
Expression column (id + 1)	correctly skipped	-

Findings

P1: The generic query-only feature is a no-op for PostgreSQL, Oracle, SQL Server and DuckDB. The default COMMENT path does not fix #11969 for these databases. Users still need table_path + query, which already worked before this PR. Either the implementation needs to resolve these drivers, or the docs/PR scope must say "MySQL-family only".

P1: ALL can merge a primary key from a join that does not preserve the table's row cardinality. SELECT t.* FROM t JOIN t2 ON ... reports every result column as originating from t, so getSinglePhysicalTablePath accepts it and ALL merges t's PK/options (JdbcCatalogUtils.java:570-573). With multiple matching rows in t2, the query output contains duplicate t rows; a sink with generate_sink_sql can switch to upsert and lose rows. COMMENT is safe, but ALL needs a join/cardinality guard or an explicit warning. The current guard checks only column origins, not the FROM/JOIN shape.

P1: SAP HANA catalog path is not aligned with the new metadata path. HANA was not run locally (no arm64 image/license), so this is code inspection, but the mismatch is concrete: SapHanaCatalog treats TablePath.databaseName as the HANA schema (SapHanaCatalog.java:94-102, 171-174; SapHanaURLParser.java:38 sets the default database to SYSTEM), while CatalogUtils.getPhysicalTablePath fills databaseName from getCatalogName and schemaName from getSchemaName. withDefaultDatabase can then fill SYSTEM while the real schema is e.g. TEST. The HANA catalog ignores schemaName, so a query on TEST.T checks SYSTEM.T or falls back. This needs a HANA-specific override or a real HANA test (at minimum, do not merge while it is untested).

P1: Real-DB coverage is still MySQL-only. JdbcMysqlTableOptionsIT is the only test that exercises the new behavior against a real database. JdbcCatalogUtilsTest/CatalogUtilsTest mock ResultSetMetaData, so they cannot detect that PostgreSQL, Oracle, SQL Server and DuckDB return different origin metadata. Please add query-only + table_path cases for at least PostgreSQL, Oracle, SQL Server, DuckDB and HANA (one per driver family), or explicitly scope and document the feature.

P2: The branch is CONFLICTING / DIRTY. Please rebase on current dev and re-run the tests; this review is against 4d4a6ecf only.

Suggested direction

1. PostgreSQL: when metadata has a table name but no schema/catalog, fill the defaults from the catalog (public / the configured database) before the early return in mergeWithUnderlyingTable, then load the table. Add a JdbcPostgresIT query-only case.
2. Oracle / SQL Server / DuckDB: their drivers do not expose getTableName() even after execution. Either require table_path for these databases (document it), or parse/resolve the query; do not rely on ResultSetMetaData alone. Add tests.
3. HANA: align the schema/database mapping or override resolveQueryTable; add a real HANA test.
4. ALL: detect joins (or at least warn) before merging PK/options, and add a join test; keep COMMENT as the safe default.
5. Docs: add a support matrix for query_table_metadata_merge per database family, and clarify that SQL Server/DuckDB catalogs do not expose table comments.
6. Rebase and re-run the new E2E matrix.

The COMMENT default is a safe incremental step and MySQL works, but I do not think the generic "preserve metadata for query-only JDBC sources" claim holds until the above is addressed.

[DanielLeens]

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.

[DanielLeens]

Thanks for making the default conservative and documenting the table_path interaction. I re-read all eighteen files at the current head and retraced both discovery paths, metadata merging and downstream planning. The NONE default addresses the unconfigured-job concern. I also rechecked @nzw921rx's real-database review rather than inheriting my earlier all-clear conclusion: its opt-in driver/cardinality concerns remain relevant. Sorry that my earlier reviews described column-origin equality as proof that a query cannot multiply rows; that was too strong. The helper-comment item below is likewise a carryover omission.

What problem this PR solves

• User pain: Query-only JDBC schemas normally omit underlying table/column comments, so automatically created destination tables lose useful metadata.
• Fix approach: Add opt-in NONE/COMMENT/ALL enrichment, prove a common reported column origin, load that physical table and merge selected metadata, with fallback on failures.
• One-line summary: Offer explicit query metadata enrichment while leaving unconfigured jobs on the previous path.

Example: query_table_metadata_merge=COMMENT can copy a MySQL table's descriptions without copying its primary key. NONE skips the additional table lookup. ALL can also copy the key, which is unsafe if SELECT t.* FROM t JOIN detail d ... duplicates rows from t.

1. Code-change review

1.1 Core logic analysis

All eighteen changed files (+1076/-91), including every production hunk, all unit tests, the changed MySQL IT/configuration and EN/ZH upgrade notes, were read. The complete target-relative diff was read at 4a08829d50173fd4f712c4691a3ba3ac6588e51e, not just the latest commit. Additive opt-in query metadata feature with a catalog resolution hook and a shared comments merge.

Before / after:

Before: query-only -> jdbcCatalog.getTable(query) / direct ResultSetMetaData -> query-derived schema.
After default NONE: same query-derived path, no underlying-table merge.
COMMENT/ALL: query table + Optional origin path -> mergeWithUnderlyingTable -> COMMENT copies comments; ALL reuses full table_path+query merge.
Origin check: every selected column reports the same catalog/schema/table; it does not parse JOINs or prove result cardinality.
OceanBase/YashanDB: shared cached connection remains open; one capped execution supplies both schema/origin data.

Complete relevant execution chain:

JdbcSourceFactory.optionRule (108) -> JdbcSourceTableConfig.of (109): per-table override, else global mode/default NONE
 -> JdbcCatalogUtils.getTables -> catalog discovery / fallback
Catalog path getCatalogTable (234-300):
  table_path+query -> existing full merge, mode ignored; table_path only -> catalog table
  query-only NONE -> existing getTable(query)
  COMMENT/ALL -> AbstractJdbcCatalog.resolveQueryTable (700)
     -> default getTable(query) + PreparedStatement.getMetaData origin probe
     -> OceanBaseMySqlCatalog.resolveQueryTable / YashanDbCatalog.resolveQueryTable: setMaxRows(1), execute once, close statement/result only
  -> CatalogUtils.getSinglePhysicalTablePath (345): all result column origins equal, empty for missing/inconsistent metadata
  -> mergeWithUnderlyingTable (549): reject empty origin / blank database+schema BEFORE loader
     -> loader withDefaultDatabase (526), tableExists/getTable
     -> COMMENT mergeComments (594): name/type-matched descriptions, keep query keys/options/identity
     -> ALL mergeCatalogTable (301): full underlying metadata for matching projection
Direct connection path (506): one dialect ResultSetMetaData -> query schema + same origin guard -> CatalogUtils table metadata loader
 -> JdbcSourceTable -> split planner reads primary/unique keys
 -> CatalogTable propagated downstream -> automatic sink DDL and JdbcSinkFactory PK selection -> generated insert/upsert writer
Errors verifying/loading -> WARN + query-derived table; missing origin itself returns unchanged table without executing a guessed SQL parser.

Key findings:

• The fresh NONE default and EN/ZH docs/test assertions restore the old unconfigured-job behavior; the earlier default COMMENT discussion is superseded.
• The table_path+query behavior is now explicitly documented and pinned by a unit test: mode NONE does not override that pre-existing full merge.
• @SEZ9's old shared-connection closure, unbounded returned-row probe, discarded verified path and DEBUG-only loader failures are addressed in the current code. The default catalog can still prepare metadata twice; it does not execute the user query twice.
• The newer @nzw921rx driver-family failures and one-sided JOIN case are not fixed by changing the default. Opted-in ALL still copies keys based only on selected-column origins.
• MySQL is the only real database exercised by the changed IT. Mock origin metadata cannot establish support for the other driver families.

Community context: +1 to @nzw921rx's CHANGES_REQUESTED review #5614398760 on the unresolved opt-in driver coverage, ALL cardinality and HANA mapping points; detailed current-head evidence is below, without duplicating them as new Daniel issue numbers. The real-database measurements are that reviewer's evidence, not tests I ran. @SEZ9's F1-F8 status requests are answered by the current paths above: shared connection/resource scope, setMaxRows(1), verified origin loading, schema-only default database, visible WARN fallback and EN/ZH mode/upgrade notes. Important qualification: default database filling occurs inside the loader, so it does not fix a table-only origin rejected before the loader. @davidzollo's older LGTM is DISMISSED, and the later explicit change request remains unresolved; I do not treat earlier approvals as current acceptance.

1.2 Compatibility impact

Compatible default, behavior-changing opt-in modes. NONE is now the default in code, per-table configuration and both language guides. Public additions are an enum/option and catalog resolution hook; no existing option/API signature is removed, and no checkpoint or protocol layout changes. COMMENT can change first-created sink descriptions; ALL may change keys/options, split planning and generated upsert/DDL. The upgrade notes record these consequences. Existing tables are not automatically rewritten. The changed Serializable table-config class has no explicit UID; no evidence in this diff establishes that it is persisted across job-version upgrades, so I am not asserting a checkpoint incompatibility from that fact alone.

1.3 Performance / side effects

The production cost and ownership implications are described below. NONE avoids the new table-enrichment lookup; OceanBase/YashanDB getTable still delegates through their bounded metadata execution. Opt-in default catalogs perform a second prepared metadata probe plus table existence/load calls at planning time. setMaxRows limits returned rows, not database query-planning/scanning cost. Column comment merging allocates name maps/new columns proportional to schema width; no per-row allocation loop, new executor, global lock, checkpoint buffer or retry mechanism is added. Resource scopes close statements/results and retain catalog-owned connections. ALL can change downstream idempotency/upsert behavior, which is why cardinality safety matters.

1.4 Error handling and logging

The existing exception and logging boundaries remain in place. Origin verification and underlying load exceptions use WARN with exception/path context and preserve the query-derived fallback. Missing/ambiguous origin is intentionally unverified rather than guessed; a support matrix must explain that behavior. No new explicit credential logging was found. NONE does not call the new enrichment loader.

+1 to @nzw921rx: ALL still equates one reported column origin with key-preserving cardinality (High, blocking; existing review #5614398760).

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/utils/CatalogUtils.java:341-369; seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:570-573; mergeCatalogTable:301-420.
• Description/risk: SELECT t.* FROM t JOIN detail d ON ... can report t as the origin of every output column while returning the same t.id multiple times. The current check accepts it and ALL merges t's primary key/options, allowing a generated-upsert sink to collapse those rows. Default NONE and COMMENT avoid that extra key merge, but do not make opted-in ALL safe. The existing generic query-key caution discusses uniqueness/splitting; it does not establish or test the safety of this added ALL route.
• Best improvement: Either conservatively reject/skip ALL when cardinality cannot be established, or explicitly limit ALL to user-asserted key-preserving queries with a clear one-sided-JOIN warning and a regression proving the chosen behavior. Do not claim all joins are detected from selected-column origins. Keep COMMENT/NONE available for such queries.
• Already raised: Yes, @nzw921rx's one-sided-JOIN P1; no duplicate Daniel number.

+1 to @nzw921rx: establish the actual driver support boundary and HANA mapping (High, required before advertising support; existing review #5614398760).

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:565-570 (blank-both early return), :288-292 (default database only in loader); seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/utils/CatalogUtils.java:375-384 (blank table origin); seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/saphana/SapHanaCatalog.java:97-101,171-176.
• Description/risk: The reviewed PostgreSQL table-only origin is rejected before default database resolution; Oracle/SQL Server/DuckDB blank table origins cannot be resolved by this implementation. These are consistent with the other reviewer's real-driver results. HANA uses TablePath.databaseName as schema, while the generic origin path separates catalog/schema; filling a default database does not remap that schema. A wrong/default schema can fail the advertised enrichment or load a same-named table from the wrong namespace. No HANA runtime result is claimed.
• Best improvement: Smallest scope: document a per-driver support/limitation matrix, require table_path where origins are unavailable, and disable/explicitly exclude unverified HANA enrichment until mapping is handled. Broader alternative: add dialect-aware resolution and real driver-family regressions, applying defaults before the early return where justified. Changing the default to NONE alone does not close the opt-in feature findings.
• Already raised: Yes, @nzw921rx's driver no-op, HANA and real-DB coverage P1s; retained as one related group, not renumbered.

Issue 9: Document the remaining physical-origin helper contract (Medium, required; carryover).

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/utils/CatalogUtils.java:372 (getPhysicalTablePath).
• Description: This new helper normalizes blank driver catalog/schema values while requiring table identity, but has no contract comment. That distinction is directly involved in the cross-driver boundary above.
• Risk: A later change can treat a missing catalog/schema as an invalid table or blindly map a dialect schema into the wrong namespace.
• Best improvement: Add a short comment documenting null-on-missing-table and catalog/schema normalization, and that dialect default/mapping is the caller's responsibility. This is a small delivery requirement, not a separate functional redesign.
• Already raised by another reviewer: No; missed by Daniel's earlier comment-completeness assessment.

2. Code-quality assessment

2.1 Code conventions

Enum modes and most new core helpers have meaningful Javadoc and new files carry ASF headers. Issue 9 covers the remaining nontrivial origin helper. The new configuration test class owns a coherent configuration-parsing unit; existing metadata utility tests and MySQL IT are extended. The scoped whitespace check produced no entries.

2.2 Test coverage and stability

Stability rating: Stable. No local build, unit/integration test, container or service was run. CatalogUtilsTest covers same/different/missing origins and SQLException fallback with deterministic mocks. JdbcSourceTableConfigTest pins default NONE, case-insensitive flat/JSON enum values and per-table override. JdbcCatalogUtilsTest covers comments versus ALL/NONE, aliases/type mismatches, loader failures, verified-path identity, schema-only defaults and table_path+query ignoring the mode. It deliberately asserts blank database+schema skips the loader, so it cannot detect/fix the reported PostgreSQL behavior. JdbcMysqlTableOptionsIT:67-76,160-168 creates real source descriptions, opts into COMMENT and verifies target SHOW CREATE TABLE plus row count; engine job exit and database health readiness are checked, with cleanup in finally. No added sleep, external-clock/random assertion or new shared port was introduced. Stability is structurally good; coverage is insufficient for non-MySQL origin behavior and one-sided JOIN cardinality.

Exact current-head fork evidence at 4a08829d50173fd4f712c4691a3ba3ac6588e51e: Build 40194756281: queued/no conclusion. Apache required-check and review gates remain independent of the fork run. Queued/in-progress/cancelled is not success; the earlier-head result does not transfer to this head.

2.3 Documentation

EN/ZH accurately set NONE by default, explain mode/table_path precedence and record opt-in consequences in incompatible-changes.md. They still lack the requested driver support matrix and explicit ALL one-sided-JOIN boundary; saying multi-table queries skip merging is too broad for origin-only detection.

3. Architectural soundness

3.1 Solution quality

A sensible opt-in enrichment boundary, with typed resolution results and comment-only mode. It is not a SQL-cardinality proof or a universally portable JDBC-origin mechanism.

3.2 Maintainability

Shared merge helpers reduce duplicate column handling; retain a clear dialect boundary instead of guessing missing namespaces. The conservative default is a useful correction.

3.3 Extensibility

resolveQueryTable is the extension point for driver-specific metadata behavior. New database support should come with real-driver evidence rather than only mocked uniform origins.

3.4 Historical compatibility

Unconfigured query-only jobs keep the previous merge mode. NONE cannot undo the pre-existing table_path+query merge or pre-existing query-key inference. COMMENT/ALL are explicit new behavior and their limitations must be documented.

4. Findings summary

No.	Finding	Location	Severity
@nzw921rx P1	ALL one-sided-JOIN cardinality can change upsert semantics	CatalogUtils.java:341; JdbcCatalogUtils.java:573	High
@nzw921rx P1	Driver support/HANA namespace boundary and real-DB coverage	JdbcCatalogUtils.java:565; SapHanaCatalog.java:97,171	High
9	Missing physical-origin helper contract	CatalogUtils.java:372	Medium

5. Merge conclusion

Conclusion: Can merge after fixes

1. Blocking items: Resolve the two existing @nzw921rx finding groups through a bounded implementation or explicit supported scope/safety contract plus regressions; add Issue 9 helper comment. Obtain passing exact-head CI and effective maintainer approval. NONE addresses default behavior but does not dismiss the other reviewer's change request.
2. Non-blocking recommendations: Avoid expanding this into a speculative SQL parser merely to recover comments. A conservative documented driver matrix and safe ALL boundary is the smaller option; dialect-aware resolution is the broader alternative.

The default and table_path clarification are good corrections. I support @nzw921rx's request to finish the opt-in safety/support boundary before merge, despite earlier dismissed LGTM records. Current-head fork Build is queued; old ClickHouse/OpenGauss failures are not current-head results and do not justify a new sync instruction.

Compare against dev: diverged, ahead 14 / behind 4, merge conflict=False. Drift is recorded independently; it does not itself justify a sync-first comment without an unrelated CI failure.

Daniel has READ permission. This COMMENT is a technical assessment, not an effective write-level approval. A write-capable maintainer must confirm repository gates and supply the effective approval.

Local evidence: exact-head fetch/checkout on seatunnel-review-11971, complete diff and source call-chain reads cited above. No runtime verification was claimed.

[yzeng1618]

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.

[DanielLeens]

Thank you @yzeng1618 for the quick and thorough turnaround. This pass is a full fresh read of the whole PR at head a89a3e35cf62 (not only the last commit), and the three things I and @nzw921rx asked for in the last round are now handled in a clear, honest way: a per-driver support matrix, a deliberate and documented ALL boundary with a merge-time WARN, and HANA excluded until its namespace mapping is aligned. The helper-contract comment I raised as Issue 9 is also in. Nothing was built, run or executed locally; this is a static review of the diff and call chains only, and I am relying on @nzw921rx's real-database measurements rather than claiming any of my own.

What Problem Does This PR Solve?

• User pain: A JDBC source defined by query only gets its schema from ResultSetMetaData, which has no column/table comments, keys or partition keys, so auto-created sink tables (Doris, StarRocks, MySQL, ...) lose that metadata.
• Fix approach: A new opt-in source option query_table_metadata_merge (NONE default / COMMENT / ALL). When the query is verified to read from a single origin table, that table is loaded and merged into the query-derived table (comments only for COMMENT, the full table_path + query merge for ALL), with a WARN and fallback on any failure.
• One-sentence summary: Opt-in metadata enrichment for query-only JDBC sources, leaving unconfigured jobs on the previous path.

1. Code Change Review

1.1 Core Logic Analysis

I re-read all 19 changed files (+1172/-91) against dev, including every production hunk, the unit tests, the MySQL IT and config, and the EN/ZH docs and upgrade notes. The delta since my last reviewed head (4a08829d50) is small: SapHanaCatalog.java, CatalogUtils.java, JdbcSourceOptions.java, QueryTableMetadataMergeMode.java, JdbcCatalogUtils.java, one new test, and the four doc files.

Before / after for the changed behavior:

Before this commit: HANA used the generic resolveQueryTable -> origin path built from
  getCatalogName/getSchemaName -> loaded under a HANA catalog that treats TablePath.databaseName as the schema
  (a query on TEST.T could end up checking SYSTEM.T).
After: SapHanaCatalog.resolveQueryTable (SapHanaCatalog.java:330) returns
  new QueryTableResolution(getTable(sqlQuery), Optional.empty()) -> mergeWithUnderlyingTable returns the query table untouched.

Before: ALL silently merged keys on any single-origin result.
After: same merge, plus log.warn at JdbcCatalogUtils.java:578 naming the underlying table and the one-sided-join hazard;
  the boundary is stated in the option description, enum Javadoc, EN/ZH Jdbc.md and incompatible-changes.md.

Runtime path (planning time, no per-row cost):

JdbcSourceFactory.optionRule -> JdbcSourceTableConfig.of (per-table override, else global, default NONE)
 -> JdbcCatalogUtils.getTables
    catalog present -> getCatalogTable (JdbcCatalogUtils.java:235)
        table_path + query -> existing full merge, mode ignored
        table_path only    -> catalog table
        query only, NONE   -> jdbcCatalog.getTable(query)   (no extra probing)
        query only, COMMENT/ALL -> AbstractJdbcCatalog.resolveQueryTable (:700)
             -> getTable(query) + origin probe via CatalogUtils.getSinglePhysicalTablePath
             -> OceanBase/Yashan override: one capped (setMaxRows(1)) execution; SapHana: no origin, merge skipped
             -> mergeWithUnderlyingTable (JdbcCatalogUtils.java:549): reject empty origin / blank db+schema
                -> withDefaultDatabase -> tableExists/getTable
                -> COMMENT: mergeComments, ALL: mergeCatalogTable
    no catalog -> direct ResultSetMetaData path (JdbcCatalogUtils.java:506), same guard and merge
 -> JdbcSourceTable (catalogTable) -> split planner / sink auto-DDL / sink PK selection
Any verification or load error -> WARN + query-derived table.

Key findings:

• The HANA override is the right minimal fix for the namespace mismatch: with an empty origin, nothing is loaded, so a same-named table in the wrong schema can no longer be merged. HANA is routed through the catalog path (it has a catalog), so the direct-connection path does not reintroduce the problem.
• The ALL approach is now a deliberate "user-asserted cardinality" contract rather than an implied guarantee. That is consistent with what I suggested last time (limit the claim, warn, pin it with a regression). COMMENT/NONE never copy keys, so they stay safe for SELECT t.* FROM t JOIN t2 ....
• CatalogUtils.getPhysicalTablePath now carries a contract Javadoc (null on missing table identity, blank catalog/schema normalized to null, defaults are the caller's job). Issue 9 from my last review is closed.
• The table_path + query behavior is unchanged and its precedence over the new mode is documented.

Community context: @nzw921rx's CHANGES_REQUESTED review (#5614398760) is on 4d4a6ecf, which predates the sync and the last two commits. I re-checked each of its points against the current head: PostgreSQL / Oracle / SQL Server / DuckDB no-op is now scoped and documented as "use table_path + query" (the smaller option that was suggested); the one-sided join is documented plus WARN (the "at least warn" option); HANA is excluded; the branch conflict is resolved. The review itself still shows as changes requested, so @nzw921rx will need to confirm. @SEZ9's F1-F8 were already addressed in earlier heads (shared-connection scope, setMaxRows(1), verified-path loading, default database, WARN fallbacks, docs).

1.2 Compatibility Impact

Fully compatible. NONE is the default in code, per-table config and docs, so unconfigured jobs follow the previous path with no extra metadata probing. The changes are additive: a new option/enum and a catalog hook (resolveQueryTable) with a default implementation; no option, API or SPI signature is removed. COMMENT/ALL are explicit opt-ins, and their effect on first-created sink DDL and (for ALL) upsert/split planning is recorded in incompatible-changes.md (EN/ZH). Existing sink tables are not rewritten. JdbcSourceTableConfig gained a field; I found no evidence in this diff that it is persisted across job-version upgrades, so I am not asserting a checkpoint compatibility concern from that alone.

1.3 Performance / Side-Effect Analysis

All new work happens once at planning time and only for opted-in query-only tables: one extra prepared-metadata probe, then tableExists/getTable. setMaxRows(1) bounds returned rows (it does not promise the database avoids planning/scan cost). No new threads, executors, locks, retries, buffers or per-row allocations. The ALL WARN is emitted once per merged table, not per row. The HANA override adds no cost.

1.4 Error Handling and Logging

Verification and load failures go to WARN with the table path and exception, then fall back to the query-derived table; missing origin is intentionally treated as "unverified" instead of guessing from SQL text. No credentials are logged. The new ALL WARN text is clear and actionable.

Issue 10: Docs say the MySQL family is "verified against real databases", but only MySQL is exercised (Low, non-blocking).

• Location: docs/en/connectors/source/Jdbc.md:56 (and the ZH mirror at docs/zh/connectors/source/Jdbc.md:56); the same wording is in the PR description.
• Problem: The matrix row lists "MySQL, OceanBase MySQL mode, TiDB, etc." as verified. The only real-database evidence is JdbcMysqlTableOptionsIT (MySQL) plus @nzw921rx's MySQL 8.0.43 run. OceanBase/TiDB are not covered by any test in this PR (OceanBase's catalog override was also changed here).
• Potential risk: Users on OceanBase/TiDB may read "Verified" as a tested guarantee and be surprised if their driver reports origin metadata differently.
• Best improvement: Reword to something like "Verified on MySQL; other MySQL-protocol databases (OceanBase MySQL mode, TiDB) are expected to behave the same because they use the same driver metadata, but are not covered by tests here."
• Severity: Low.
• Raised by another reviewer: No.

Issue 11: The new HANA exclusion has no direct unit test (Low, non-blocking).

• Location: seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/saphana/SapHanaCatalog.java:330.
• Problem: The override is a 2-line guard against loading a same-named table from the wrong schema, but nothing asserts that it returns an empty origin. If someone later removes it (or the base class changes), the HANA wrong-schema risk @nzw921rx described returns silently.
• Potential risk: Regression of a data-correctness guard in an opt-in path.
• Best improvement: One small unit test (mock SapHanaCatalog.getTable(String) and assert resolveQueryTable(...).getSinglePhysicalTablePath() is empty). No real HANA instance is needed for this guard.
• Severity: Low.
• Raised by another reviewer: No.

2. Code Quality Assessment

2.1 Coding Standards

New enum, option, helpers and the HANA override have meaningful Javadoc/comments that explain why (namespace mapping, cardinality assumption), and new files carry ASF headers. Public surface stays small. Two comments are worth keeping in sync if the support matrix changes: the option description in JdbcSourceOptions.java and the getPhysicalTablePath contract.

2.2 Test Coverage and Test Stability

Rating: Stable. Unit tests cover single-origin resolution, join/expression/missing-origin rejection, metadata-failure fallback, verified-path loading, schema-only withDefaultDatabase, table_path + query ignoring the mode, mode defaults and per-table override. The new testAllModeMergesOneSidedJoinOriginPerUserAssertion (JdbcCatalogUtilsTest.java:830) pins the deliberate ALL boundary: it fails if the loader is skipped or the merged primary key stops coming from the underlying table, so a later semantic change shows up as a visible test diff. No sleeps, fixed ports, shared static state or timing assertions were added; the MySQL IT (JdbcMysqlTableOptionsIT) verifies comments through SHOW CREATE TABLE of the auto-created sink table. Coverage gap (acknowledged by the author as future work): non-MySQL real-driver behavior and a HANA guard test (Issue 11). The mocks cannot prove any driver's origin metadata, which is why the docs now say so.

Current head CI: the apache Build check is still in progress (https://github.com/apache/seatunnel/runs/121424833630); there is no completed result for this head yet, and earlier-head results do not transfer.

2.3 Documentation Updates

EN and ZH Jdbc.md and incompatible-changes.md are updated and consistent with the code: default NONE, mode semantics, table_path precedence, the per-driver matrix (MySQL supported; PostgreSQL / Oracle / SQL Server / DuckDB not resolved; HANA excluded) and the ALL one-sided-join warning. Only the "Verified" wording in Issue 10 needs tightening.

3. Architectural Soundness

3.1 Solution Quality

A reasonable, bounded opt-in enrichment. It is honest that origin-column checks do not prove row cardinality and that drivers differ.

3.2 Maintainability

resolveQueryTable plus the typed QueryTableResolution keep driver-specific behavior inside the catalogs (OceanBase, Yashan, HANA) instead of the shared merge helper, and the merge logic reuses mergeCatalogTable.

3.3 Extensibility

Adding PostgreSQL/Oracle/SQL Server/DuckDB later fits the same hook (dialect-aware origin resolution plus real-driver ITs), as the author notes in the PR discussion.

3.4 Historical Compatibility

Default NONE keeps pre-existing behavior; table_path + query is untouched; the new modes are explicit and documented.

4. Issue Summary

No.	Issue	Location	Severity	Blocking	Raised by another reviewer
10	"Verified against real databases" overstates MySQL-family coverage	docs/en/connectors/source/Jdbc.md:56 (and ZH)	Low	No	No
11	HANA exclusion has no unit test	SapHanaCatalog.java:330	Low	No	No
-	@nzw921rx P1s (driver no-op, ALL join, HANA, MySQL-only coverage)	various	-	Addressed as scope/documentation in this head; reviewer confirmation pending	Yes

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers: No code-level blockers from my side at this head. Remaining gates are not code: the apache Build check on this head must finish green, @nzw921rx's change request (made against the older 4d4a6ecf) needs their confirmation or dismissal, and a write-capable maintainer must give the effective approval (I only have read permission, so this COMMENT is a technical assessment, not a merge approval).
2. Recommended non-blocking fixes: Issue 10 (soften the "Verified" wording for OceanBase/TiDB) and Issue 11 (one tiny HANA guard unit test).
3. Overall assessment: The feature is now safely opt-in, scoped to what has real evidence, and honest about its limits. The follow-ups the author lists (dialect-aware resolution for the other drivers, HANA namespace alignment, a structural join guard) are fine as separate work.
4. Better alternative: None needed for this PR; for the later driver work, resolving origin per dialect with real-driver ITs is the right direction rather than parsing SQL text.

Compare against dev: diverged, ahead 15 / behind 8, mergeable (no conflict). Being behind is not by itself a reason to sync.

[DanielLeens]

Thanks for the contribution. The merge-gate fact cited in my previous conclusion has changed: GitHub now reports a real merge conflict. This follow-up corrects that gate on the unchanged reviewed head; it is not another full source review or a sync request based only on being behind.

Gate snapshot (2026-10-28T15:57:39+00:00), head a89a3e35cf620e64feba069964a23e9b3d33a438: Build is FAILURE.

The blocking jobs are Run / updated-modules-integration-test-part-8 (11, ubuntu-latest) failure.

The failed job list is observed, but its causal error is not established by the available summary. Please inspect the linked failed step/report before choosing a source change; no verified upstream sync-only cure is established in this follow-up.

My previous source conclusion remains clear for the reviewed scope. CI is not being passed by inference, and this follow-up is not an effective approval.

Separately, GitHub reports merge conflicts with dev. Please sync the latest base and resolve the conflicts first; the resulting new head needs checks and re-review. This conflict gate is independent of CI and existing source findings.

[DanielLeens]

Thanks for the new head and the HANA regression test. I reviewed the complete 20-file diff at aaefaa9, including catalog/no-catalog discovery, the runtime and dry-run callers, merge semantics and the MySQL job fixture. The source blockers raised earlier are addressed within the explicitly documented scope. Build is now green, but nzw921rx's requested-changes review still needs reconciliation by that reviewer/a write-capable maintainer. One documentation follow-up remains non-blocking.

What Problem Does This PR Solve?

User pain point: A query-only JDBC source may omit table and column comments from its produced schema, so an automatically created destination table loses those comments.

Fix approach: Add an explicit NONE/COMMENT/ALL metadata-enrichment option. Verify that every result column reports the same physical origin, load that verified table and merge the selected metadata, with a query-derived fallback when enrichment cannot be performed.

One-sentence summary and example: With query_table_metadata_merge=COMMENT, a MySQL SELECT from a single table can carry its comments into a newly created destination while retaining the query schema's existing keys and options.

1. Code Change Review

1.1 Core Logic Analysis

JdbcSourceTableConfig inherits the top-level mode unless a table_list entry overrides it; the default is NONE. Both JdbcSource construction and JdbcSourceFactory.inferSchemaForDryRun use JdbcCatalogUtils.getTables. Explicit table_path takes the existing path/full-merge behavior before consulting the new mode. Query-only catalog discovery uses getTable directly for NONE, otherwise resolveQueryTable plus mergeWithUnderlyingTable. No-catalog discovery reuses one ResultSetMetaData for schema and origin checking.

Before:

return jdbcCatalog.getTable(tableConfig.getQuery());

After:

if (metadataMergeMode == NONE) {
    return jdbcCatalog.getTable(query);
}
QueryTableResolution result = jdbcCatalog.resolveQueryTable(query);
return mergeWithUnderlyingTable(result.getTable(), result.getSinglePhysicalTablePath(), loader, metadataMergeMode);

Key Findings

• Default NONE is wired through the option, top-level/table_list parsing, enum conversion and tests; the factory exposes the option.
• table_path plus query ignores the new mode, including NONE, and retains the existing full merge.
• The loader receives the verified origin path rather than a separately inferred query identifier; schema-only catalog paths receive the catalog default database.
• A table-only origin with neither database nor schema is deliberately rejected before lookup. This explains the documented PostgreSQL limitation; it is not silently claimed fixed by default-database filling.
• HANA explicitly returns an empty verified origin because its namespace mapping differs, with a direct regression test.
• OceanBase and Yashan reuse one executed query result and cap returned rows to one. Yashan no longer closes the cached shared connection.
• COMMENT retains query column order, type/length/nullability/defaults/options and keys while copying matching-name/SqlType comments. ALL reuses the existing full merge, which can also use physical column definitions and identity when its schema-match branches apply.

Complete runtime path

source config / table_list -> JdbcSourceTableConfig -> JdbcSourceConfig
 -> JdbcSource or dry-run schema inference -> JdbcCatalogUtils.getTables
 -> explicit table_path: existing behavior
 -> query-only NONE: existing query-derived table
 -> query-only COMMENT/ALL: resolve origin -> load exact table -> scoped merge
 -> produced CatalogTable -> split planning + downstream sink schema/DDL/SQL

The origin check proves only that selected columns report the same table; it cannot prove join cardinality. ALL is an explicit user assertion, emits a warning and documents the one-sided-join hazard. Its test pins that boundary. Missing origins/expression columns skip enrichment for the whole query. COMMENT does not introduce underlying-table keys, but it also does not remove primary keys already inferred by the pre-existing catalog path. Therefore it is not a general cure for unsafe query-key inference, which the existing guide separately explains.

1.2 Compatibility Impact

Fully compatible. For existing configuration, NONE preserves the prior merge choice; existing table_path behavior and the query-derived row schema are retained. New modes are opt-in, with per-table override. No existing option, factory/SPI registration or public method is removed. Added catalog hooks are implemented by default. The OceanBase/Yashan refactoring still checks origin metadata internally even through their getTable wrapper, but does not load/merge the underlying table in NONE mode. Returned-row capping and corrected cached-connection ownership are beneficial planning behavior changes, not proof of identical database work. No persisted split/checkpoint format changes here.

1.3 Performance / Side-Effect Analysis

Enrichment runs at discovery, not per row. The generic catalog implementation still performs a second prepared metadata probe after deriving the query table; it does not execute the user query again through that new probe. The executing OceanBase/Yashan overrides share one result. setMaxRows(1) caps returned rows but does not guarantee bounded database scan/planning time. The default does not perform the new underlying-table load. No new thread, queue, retry loop or unbounded in-memory result buffer is introduced.

1.4 Error Handling and Logging

Missing/unusable origin is a deliberate no-op. Verification SQL exceptions and underlying-table load failures warn and retain the query-derived schema. The new ALL warning states the cardinality/upsert consequence. Failures in deriving the original query schema still follow existing discovery/multi-table failure handling; they are not hidden by the enrichment fallback. Statements/results close independently of the catalog-owned connection.

Issue 1: Finish tightening the metadata and verification claims in documentation

• Location: docs/en/introduction/concepts/incompatible-changes.md: JDBC query_table_metadata_merge entry; Chinese mirror; docs/en/connectors/source/Jdbc.md: query-only support paragraph
• Problem description: The connector support-table row now correctly says only MySQL was tested, but the migration entry still describes MySQL-family drivers as verified. It also describes the old query-derived schema as carrying no keys, while CatalogUtils.getCatalogTable(Connection,String,JdbcDialectTypeMapper) already invokes extractPrimaryKey. The paragraph saying NONE/COMMENT stay safe for such joins should be limited to the new enrichment path, since these modes retain any pre-existing inferred key.
• Potential risk: These statements can overstate tested driver coverage or imply that selecting NONE/COMMENT removes an already inferred key. The guide's separate query-key caution helps, and this is a documentation precision issue rather than a new source blocker.
• Best improvement: Carry the MySQL-only verified wording into both migration notes. Say NONE preserves the existing query-derived schema, including any existing key inference; COMMENT adds only comments and does not cure pre-existing join/key inference. Keep the existing caution link.
• Severity: Low; non-blocking.
• Raised by another reviewer: Partly: continues the earlier Daniel MySQL-only evidence wording request; the existing-key clarification follows the current source audit.

2. Code Quality Assessment

2.1 Coding Standards

The new origin resolver, merge modes, comment-only helper, HANA exclusion and shared-connection ownership have meaningful contracts/comments. Driver exceptions stay in catalog implementations; shared merging operates on a verified path and an explicit mode. The comment helper extraction preserves its former field-copy logic.

2.2 Test Coverage and Test Stability

Test stability: Stable. Unit additions use deterministic local mocks/data for origin agreement, joins selecting both sides, expression/missing origins, failure fallback, exact-path loading, default database handling, mode parsing/precedence, unchanged table_path behavior and the explicit ALL cardinality assumption. The new HANA test pins the exclusion without claiming a real HANA connection. The existing MySQL container/job fixture now enables COMMENT and asserts both column comments and table comment through SHOW CREATE TABLE, alongside row count and existing table-option assertions; destination cleanup remains in finally. No new sleep, fixed host port, shared global mutation or clock-dependent expectation is introduced. Tests for non-MySQL live driver families remain outside this PR's evidenced support scope. No local tests/containers were run.

Local analysis: fetched the head, created independent branch seatunnel-review-11971, read the complete 20-file diff and traced affected callers with git show/git grep. No local compilation, unit tests, integration tests, E2E, containers, or job execution was performed, as requested. These are source-analysis conclusions; execution evidence is the current-head CI below.

2.3 Documentation Updates

Both connector guides and migration notes describe default NONE, opt-in semantics, per-table overrides, explicit table_path precedence and the support matrix. MySQL is the real-database evidence; other MySQL-protocol products are expected, not individually validated here. PostgreSQL/Oracle/SQL Server/DuckDB require the documented explicit table_path path, and HANA query-only enrichment is excluded. Issue 1 records the remaining wording corrections.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. The source-discovery boundary is correct because it owns the CatalogTable before splitting and downstream DDL generation. The loader receives the verified origin and driver-specific resolution stays in catalog overrides. The smallest safe scope is explicit enrichment with honest driver limits. A global cross-driver SQL/cardinality resolver would require dialect-aware parsing and real-driver matrices; adding that here would substantially expand compatibility and verification costs. It should be separately designed, rather than guessed from a default schema.

3.2 Maintainability

One typed resolution object keeps the schema and verified path associated; the fallback path reuses metadata. Generic catalogs still use two prepared probes, so this is not a universal single-probe guarantee. Keep the HANA exclusion until its namespace mapping is explicitly aligned and tested.

3.3 Extensibility

Future database-family support belongs in the catalog resolution hook with actual driver-origin evidence. Before enabling ALL for more query shapes, preserve the distinction between column origin and cardinality; a successful metadata lookup alone is insufficient.

3.4 Historical-Version Compatibility

No automatic metadata merge is enabled for old jobs. NONE preserves their existing key inference rather than promising no primary key. Existing destination tables are not rewritten by comment enrichment. This review does not claim executed upgrade/restore tests or general multi-database acceptance.

4. Issue Summary

Number	Issue	Location	Severity
1	Finish tightening the metadata and verification claims in documentation	docs/en/introduction/concepts/incompatible-changes.md: JDBC query_table_metadata_merge entry; Chinese mirror; docs/en/connectors/source/Jdbc.md: query-only support paragraph	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed
   • No source blocker identified. No blocking source finding or current-head CI failure remains. Reconcile the still-active nzw921rx CHANGES_REQUESTED gate and obtain effective maintainer approval; this COMMENT cannot dismiss it. Issue 1 is non-blocking.
2. Recommended fixes — non-blocking
   • Issue 1: Finish tightening the metadata and verification claims in documentation

Overall assessment: The opt-in design and narrowed support claim address the earlier correctness/compatibility objections, and the HANA guard now has regression coverage. Repository review state remains a separate merge gate.

Existing-review reconciliation: +1 to nzw921rx's real-database evidence and explicit alternatives: the implementation chose documented driver limits, HANA exclusion, default NONE and a warned/tested user assertion for ALL. The inline requests about NONE and table_path precedence are answered in code, tests and docs. +1 to SEZ9's cached-connection, repeated execution, exact origin-path and fallback-visibility findings: those blocking paths are corrected; the generic second prepared probe remains an acknowledged non-blocking cost. My previous missing-HANA-test request is resolved. The support-table wording request is fixed there but not fully in the migration notes, as consolidated in Issue 1. The old davidzollo approval is DISMISSED; no current independent approval is inferred. Author replies have been incorporated into this single full review.

Head aaefaa92d0b8755dca013e99095c3d24f5eb4f5a. Live base dev at 3ac2d1cec6aa7b3741c4dacdd23020f778e23c02; comparison ahead, ahead 17 / behind 0; merge state BLOCKED. Behind/diverged alone is recorded metadata, not a request to synchronize upstream. Checks: Signal pull request update = SUCCESS; Build = SUCCESS

Fresh current-head Build and PR signal are SUCCESS. Build replaces the older failed-head check. GitHub reports BLOCKED / CHANGES_REQUESTED, rather than a newly verified merge conflict. The old conflict-only sync request is no longer a current action item; ahead/behind metadata alone does not justify another synchronization. No retry was triggered.

Daniel has READ repository permission. This COMMENT records the technical conclusion; it is not an effective maintainer approval or a dismissal of any historical requested-changes entry. A write-capable maintainer owns the final repository approval gate.

[DanielLeens]

Thanks for the new head 38b2617602d2afcab6c0b7e3d01e84b903f6157e. Since my last review (aaefaa92) the delta is documentation only: four files (en and zh Jdbc.md and incompatible-changes.md). No Java or test file changed, so the source analysis from my previous full review of the 20-file diff still applies unchanged. Re-verified this round: the file list of the delta, and the doc wording against the points I raised.

What Problem Does This PR Solve?

User pain point: A query-only JDBC source derives its schema from ResultSetMetaData, so column and table comments are lost and auto-created sink tables (Doris, StarRocks, MySQL, ...) have no comments.

Fix approach: Add an opt-in source option query_table_metadata_merge (NONE default, COMMENT, ALL). When every selected column reports the same origin table, load that table and merge the selected metadata into the query-derived table; otherwise skip the merge.

One-sentence summary: With query_table_metadata_merge=COMMENT, a single-table MySQL query keeps its schema but now carries the underlying column and table comments to an auto-created destination.

1. Code Change Review

1.1 Core Logic Analysis

Unchanged from the last review: explicit table_path keeps the existing full merge and never consults the new mode; query-only NONE calls getTable(query) as before; COMMENT/ALL go through resolveQueryTable and a merge on the verified origin path. HANA returns an empty verified origin and has a regression test.

Before:

return jdbcCatalog.getTable(tableConfig.getQuery());

After:

if (mode == NONE) { return jdbcCatalog.getTable(query); }
// else resolve verified origin table, load it, merge per mode

source config / table_list -> JdbcSourceTableConfig -> JdbcCatalogUtils.getTables
 -> table_path set: existing behavior
 -> query only + NONE: existing query-derived table
 -> query only + COMMENT/ALL: verify single origin -> load table -> scoped merge -> split planning and sink DDL

The doc delta resolves my only open item (previous Issue 1): the migration notes (en and zh) now say MySQL was verified against a real database and other MySQL-protocol drivers are expected but not covered; they state that the default NONE keeps the pre-existing key inference of the query-derived table; and the join caveat in both guides is scoped to the new enrichment path, noting that COMMENT/NONE do not remove a key already inferred by the existing catalog path.

1.2 Compatibility Impact

Fully compatible. Default NONE preserves existing behavior; new modes are opt-in with per-table override; no option, SPI or factory registration is removed; no split/checkpoint format change. This head changes no code.

1.3 Performance / Side-Effect Analysis

Unchanged: only opt-in modes add a metadata probe; ALL logs a warning about the one-sided-join cardinality assumption.

1.4 Error Handling and Logging

No new findings for this head. The previous documentation item is closed.

2. Code Quality Assessment

2.1 Coding Standards

No code change in this head.

2.2 Test Coverage and Test Stability

Test stability: Stable. No test changed in this head. The tests reviewed last round (origin agreement, joins, expression columns, HANA exclusion, mode precedence, the pinned ALL assumption, and the MySQL IT asserting column and table comments) are unchanged. I did not run anything locally.

2.3 Documentation Updates

en and zh are updated consistently in this head and match the support matrix in the connector guides.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix with honest driver limits; a cross-driver SQL/cardinality resolver stays out of scope.

3.2 Maintainability

Keep the HANA exclusion until its namespace mapping is aligned and tested.

3.3 Extensibility

New driver families belong in the catalog resolution hook with real driver evidence.

3.4 Historical-Version Compatibility

No automatic merge for old jobs; existing sink tables are not rewritten.

4. Issue Summary

Number	Issue	Location	Severity
-	No open source or documentation issues on this head	complete diff	-

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers — must be fixed
   • No source blocker. Two non-source gates remain: the current-head Build was still in progress when I checked, so I cannot call CI green yet; and nzw921rx's CHANGES_REQUESTED review (on the older head 4d4a6ecf) is still active. Their concerns (MySQL-only evidence, ALL join cardinality, HANA mapping, default NONE, table_path precedence) were answered with a documented support matrix, a warned and test-pinned ALL assumption, the HANA exclusion and the NONE default. @nzw921rx, could you re-check this head and update your review state? A write-capable maintainer also needs to approve; Daniel only has READ permission.
2. Recommended fixes — non-blocking
   • None.

Overall assessment: The opt-in design, the narrowed and documented driver scope and the corrected doc wording close the earlier concerns. The branch shows 9 commits behind dev with no conflict reported; I am not asking for a sync, since Build is still running on this head.

Existing-review reconciliation: +1 to nzw921rx's real-database evidence and to SEZ9's earlier cached-connection and exact-origin findings, which are fixed in code. The old davidzollo review is DISMISSED.

[DanielLeens]

CI status update for head 38b2617602: when I last reviewed this head, the current-head Build was still running, so I could not call CI yet. That run has now finished with a failure, so the CI fact in my previous comment is out of date. My source-side conclusion is unchanged.

What failed in the fork run (https://github.com/yzeng1618/seatunnel-dev/actions/runs/41381345787): Run / unit-test (17, ubuntu-latest) and Run / updated-modules-integration-test-part-2 (11, ubuntu-latest). Specifically, in unit-test, the only error is JobStateEventTest.testNotEndJobStateEvent:173 (Awaitility ConditionTimeout, seatunnel-engine-server); in updated-modules-integration-test-part-2 every JDBC E2E class that finished passed (for example JdbcOracleIT 39/39, JdbcMariaDBIT, JdbcDb2IT, JdbcAutoGenerateSQLIT, JdbcSinkBatchIntervalIT), and the log stops mid-run without a test failure, which matches the job hitting its time budget rather than an assertion failing.

Neither failure points at the JDBC source change in this PR: the unit-test error is an engine-server event test, and no JDBC IT failed. The branch already contains the recent upstream CI fixes, so a sync with dev is not expected to help.

The lowest-cost next step is to re-run only the failed jobs in your fork run ("Re-run failed jobs" on the run page). If the same test fails again on the re-run, please paste the new run link here and I will help narrow it down; please do not add unrelated changes to this PR just to chase it. Thanks again for your patience!