DanielLeens commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5645072535
@SEZ9 @luozihen I independently re-pulled the head (`81f97896`) and checked F1 against both files directly rather than the status summary. **`ReadonlyConfig#toConfig()`** — byte-for-byte identical to `dev` (base `1a8c637e996e`): `diff` between the two produces no output. So the API-level surface really is untouched, not just "functionally equivalent." **`MultiTableFailureHelper.mergeOptions()`** — confirmed it no longer round-trips through HOCON: ```java // dev: return ReadonlyConfig.fromConfig(primary.toConfig().withFallback(fallback.toConfig())); // this PR: Map<String, Object> merged = new HashMap<>(); merged.putAll(fallback.getSourceMap()); merged.putAll(primary.getSourceMap()); return ReadonlyConfig.fromMap(merged); ``` On your ask — whether a key present in both sides resolves the same way as before — there's a real semantic difference worth naming precisely, not just "looks fine": the old `Config#withFallback` merges nested objects recursively (if both sides have an object at the same path, sub-fields from both survive, primary's sub-fields win on conflict), while the new code does a **shallow top-level merge** — if the same top-level key holds a nested map on both sides, `primary`'s whole nested value replaces `fallback`'s wholesale, and any of `fallback`'s sibling sub-fields not present in `primary` are silently dropped. So this is a real narrowing of `mergeOptions()`'s contract, but I traced every current caller and none of them can actually hit the diverging case: - `MultipleTableJobConfigParser.java:746`, and the Spark (`seatunnel-spark-2-starter`/`seatunnel-spark-starter-common`) and Flink (`seatunnel-flink-13-starter`/`seatunnel-flink-starter-common`) `SinkExecuteProcessor`s all call it as `mergeOptions(sinkConfig, envConfig)` — sink-plugin options and job-env options are disjoint namespaces; there's no key a JDBC/any sink config would set that env config also sets as a nested object. - `withMultiTableFailurePolicy`/`withFailedTables` call it as `mergeOptions(singleKeyMap, options)`, where `singleKeyMap` only ever contains `MULTI_TABLE_FAILURE_POLICY.key()` or `MULTI_TABLE_INITIAL_FAILED_TABLES.key()` — neither collides with anything a sink config would set. - `MultiTableFailureHelperTest#testMergeOptionsPreservesSpecialCharacterKeys` covers `multi_table_config` present only on the `primary` side plus a genuinely overlapping scalar key (`url`, primary wins) — it doesn't exercise the "same key nested on both sides" case, because no real caller produces that input today. So F1 holds for every actual code path in the repo right now. The one thing I'd ask for, since this is a shared helper used by three engines (Zeta, Spark, Flink), not just this PR's JDBC feature: a one-line doc comment on `mergeOptions()` stating the merge is shallow/top-level (primary replaces fallback's value wholesale on key collision, not a recursive object merge) so a future caller doesn't assume HOCON-style deep merge like the old implementation had. Not a blocker on my side — just want that contract written down rather than implicit, given how many call sites depend on it. -- 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]
