SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6356176698
@luozihen thanks for digging through the CI logs.
**Issue 1 (dead link):** I cannot tell from the thread whether that URL
comes from the base branch or from the JDBC sink doc this PR touches. Could you
check whether the link appears in your diff? If it is only in the base branch,
a rebase onto the latest main should tell us, and a docs-only fix in a separate
PR would be the right place for it; I would not block this PR on it. If it is
in your diff, please update or remove the link here.
**Issue 2 (`ZetaSQLEngine - Open udf failed` in unit-test):** re-triggering
is fine; I do not expect a code change from you for it unless it reproduces
consistently.
On the `ReadonlyConfig.toConfig()` key-contract thread, a few concrete asks
to close it out:
1. I lean the same way you do: quote only the keys that fail path parsing,
so existing keys keep their current behaviour and only keys that cannot be
represented today are rescued.
2. Only one PR should carry the shared conversion change. If it lands
elsewhere, please drop `0c43f10dd` from this PR and rebase once that is merged;
if it lands here, keep it as the single self-contained commit it is now.
3. Please add the REST-shaped test you described (map ->
`RestUtil.buildConfig` -> `ConfigShadeUtils.decryptConfig` with a regex key,
`job.mode`, `shade.identifier` and `a->b`), asserting which keys stay literal
and which still expand. Since that exercises the REST path rather than
`ReadonlyConfig.toConfig()` directly, please also add a direct `toConfig()`
test for the dotted-key behaviour so both paths are pinned.
The JDBC-specific items from the earlier review (declaration-order guarantee
for the map-typed option and its docs, identifier validation on the resolved
key columns, the `${primary_key}`/`${unique_key}` vs. `TablePlaceholder`
interaction, regex compile-time validation mapped to JDBC-12, and the
`multi-table_config` naming) are independent of the shared conversion, so feel
free to push those regardless of which PR carries the config change.
<!-- streview-comment:1963 -->
--
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]
