DanielLeens commented on code in PR #11971:
URL: https://github.com/apache/seatunnel/pull/11971#discussion_r4331841818
##########
docs/zh/connectors/source/Jdbc.md:
##########
@@ -157,6 +159,7 @@ driver = "org.mariadb.jdbc.Driver"
| username | String | 否 | - |
数据库账号。历史配置键 `user` 仍作为兼容键支持。
|
| password | String | 否 | - |
数据库账号的密码。
|
| query | String | 否 | - | 要执行的
SQL。需要同时明确表身份和元数据时，可以与 `table_path` 组合使用。
|
+| query_table_metadata_merge | Enum | 否 | COMMENT | 仅配置
`query` 的源表合并底表元数据的范围（仅在查询被验证为映射到单张物理表时生效）。`COMMENT` 仅合并字段注释和表注释；`ALL`
额外合并表选项、主键、约束键和分区键——效果等同于同时配置 `table_path` 与 `query`，可能改变 Sink 建表 DDL，使配置了
`generate_sink_sql` 的 Sink 从 insert 切换为 upsert，并启用基于主键的切分规划；`NONE`
完全跳过底表解析。也可以在 `table_list` 中按表单独配置。
|
Review Comment:
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.
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/JdbcSourceFactory.java:
##########
@@ -95,6 +95,7 @@ JdbcSourceOptions.TABLE_LIST, new
TableListExclusiveValidator()))
JdbcSourceOptions.STRING_SPLIT_MODE_COLLATE,
JdbcSourceOptions.PROPERTIES,
JdbcSourceOptions.QUERY,
+ JdbcSourceOptions.QUERY_TABLE_METADATA_MERGE,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
