DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6414884620
Hi @luozihen, thanks for the clear plan, and for telling everyone before
holding the branch.
This sounds right to me, and it matches the direction we converged on:
- #12443 ("Preserve literal config keys across REST and file loading") is
still open, on head `509fddab`; I approved it separately. Landing the
config-key conversion there first, so this PR does not carry a competing one,
is the safest order.
- After #12443 merges, syncing `dev` and dropping `0c43f10dd` (the
`ConfigBuilder.of(Map)` commit) removes the compatibility concern I raised on
the REST path. The sync should also clear the unrelated `LLMBooleanOutputTest`
failure in the current Build, which is already fixed on `dev` (#12938).
- Please keep the REST-shaped regression (`map -> RestUtil.buildConfig ->
ConfigShadeUtils.decryptConfig`) and the `toConfig()` assertions, rebased on
top of #12443's contract, with `shade.identifier`, `job.mode`, `a.b`, `a->b`
and a regex key asserted together, so the test pins the final behavior. If the
Jdbc docs (en and zh) still claim that regex keys work for the REST JSON body,
please keep that wording consistent with whatever #12443 actually delivers.
- The JDBC `multi_table_config.primary_keys` feature and its validation stay
as they are.
The head is still `d9bb1574`, so my existing review stands for now and I
cannot mark the REST point as resolved yet. Once you post the rebased head, I
will do a full re-review of it and update my review state accordingly. No
action is needed from you until #12443 lands.
--
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]
