SEZ9 commented on PR #11493:
URL: https://github.com/apache/seatunnel/pull/11493#issuecomment-5358313724
Thanks for the thorough re-verification pass, @DanielLeens. Given the head
is unchanged at `8ddb8ff420f0`, I agree there's no need to re-post the full
review — but the four previously raised findings still stand as merge blockers,
and your re-confirmation of the silent-behaviour defects reinforces that.
Concretely, here is what I still need before this can move forward:
1. **PR11493-F1 (HIGH, Compatibility):** The silent STRING → JSON mapping
change for Snowflake `VARIANT`, Doris `JSON`/`JSONB`, and StarRocks `JSON`
source columns needs an explicit opt-out (e.g., a config flag to retain the
legacy STRING mapping) plus an entry in
`docs/en/introduction/concepts/incompatible-changes.md`. Upgrading users must
not have pipelines break with no escape hatch.
2. **PR11493-F2 (Logic):** Since the JSON logical type is physically a plain
String, please clarify (and document in
`docs/en/introduction/concepts/schema-feature.md`) where validation happens. If
validation only exists at the StarRocks load boundary, we need a stated
strategy for the other native-JSON sinks, or a documented contract that
malformed text is the user's responsibility.
3. **PR11493-F3 (Robustness):** The StarRocks JSON-column + default CSV
Stream Load combination should be rejected at sink initialization, not at load
time. Please add fail-fast validation and update
`docs/en/connectors/sink/StarRocks.md` accordingly.
4. **PR11493-F4 (Robustness):** The Snowflake sink silently skipping
`PARSE_JSON` wrapping when a user-defined query is supplied defers failure to a
mid-run server error. Either fail fast at init with a clear message, or
document the limitation prominently in `docs/en/connectors/sink/Snowflake.md` —
silence is not acceptable here.
Additionally, you noted that two of this PR's own new E2E suites are red —
those need to be green before merge, so please push the fixes for the above and
let CI re-run.
One housekeeping note: your comment appears to have been cut off
mid-sentence ("every connector/format that switches on..."). Could you re-post
the remainder so nothing from your re-verification is lost?
No new scope from my side — resolving F1–F4 and getting the E2E suites
passing is the full remaining list.
<!-- streview-comment:406 -->
--
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]
