DanielLeens commented on PR #11493: URL: https://github.com/apache/seatunnel/pull/11493#issuecomment-5363709724
Thanks @SEZ9 — head is still `8ddb8ff420f0`, no new commit, so this is a reply, not a new full review. **Housekeeping note first: my previous comment was not cut off.** The "every connector/format that switches on..." sentence you flagged is inside the fenced runtime-path code block near the top of that comment, and it's a complete sentence: "...every connector/format that switches on SqlType must carry an explicit `case JSON:` or the value silently falls into whatever the `default:` branch does for that site -- and Java does not fail compilation when an enum gains a new constant, so an un-updated switch is a *runtime*, not *build-time*, gap." The comment continues normally after that (into "### Independently re-verified findings...") and ends cleanly at the "Upstream sync note (informational, not a blocker rationale)" paragraph at the very end. Nothing is missing — I've re-read the rendered comment top to bottom to confirm. Apologies if the code-fence formatting made it look truncated. On F1–F4: all four are already on my own tracked list, so there's no daylight between us on whether they need to be resolved before merge — F2 maps to my Issue 2, F3 to my Issue 3 (StarRocks CSV/JSON fail-fast) and Issue 13, F4 to my Issue 14 (Snowflake `PARSE_JSON` inconsistency). I'll fix all three as part of the same pass. **F1 is the one place I still owe you a clear answer, so here it is plainly: my position is unchanged from my last round, and I still don't think a `json.type.mapping` opt-out flag is the right fix, for the same reason I gave on 2026-08-13** — this project already merged an unconditional, opt-out-free source-type remap with only a documented migration path (the "Breaking Change: Mapping of timezone-aware timestamp columns to TIMESTAMP_TZ type" entry in `docs/en/introduction/concepts/incompatible-changes.md`, which changed MySQL/PG/Oracle/SQL Server/Snowflake timestamp mappings unconditionally). If that bar was acceptable for merge, I don't think this PR should be held to a stricter bar just because it's a second instance of the same pattern. But — and this is the part that actually blocks merge right now — my own 2026-08-19 re-verification pass found that the documented "blast radius" for this remap is provably incomplete, independent of whether an opt-out flag exists: - Doris-source → SelectDB-Cloud-sink pipelines hard-fail after this change (Issue 6), and SelectDB Cloud isn't listed as affected anywhere in the incompatible-changes doc. - The Qdrant sink silently drops JSON columns with zero error/log/metric (Issue 4) — not merely "unsupported," but silent data loss, and Qdrant is also unlisted. - `seatunnel-format-text`, and therefore Doris's own default CSV Stream Load, has no `case JSON:` (Issue 8), so the Doris source now emits a type its own CSV sink path can't write — also unlisted. So the honest framing is: I disagree that F1 *requires* an opt-out flag to merge, but I agree it currently blocks merge — just for a narrower reason than "no opt-out exists." The remap can ship as a documented breaking change the same way the timestamp one did, but only once the "supported components" list in `incompatible-changes.md` is actually complete and every entry on it is true. Right now it isn't, on three counts above. If, once those three are fixed, you still want a hard opt-out rather than a complete/accurate doc, I'm open to hearing the case again — but I don't think we're there yet on the facts. Net: this PR is not close to a clean re-review pass yet. On top of F1–F4, the two CI-red E2E suites (`LocalFileIT`, `DorisIT.assertJsonColumnsRoundTrip`) and the silent `ARRAY<JSON>`→`ARRAY<STRING>` downgrade in `CalciteTypeConverter` are still unfixed, deterministic, and — per my last pass — reproduced fresh on a rerun with no intervening commit, so they're not CI noise. I'll work through the full list (CI reds, Calcite, Qdrant, SelectDB Cloud, format-text, plus F2–F4) together rather than piecemeal, and will ping this thread once a real commit lands so both of us can re-review the actual diff instead of re-confirming the same head again. -- 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]
