SEZ9 commented on PR #11493: URL: https://github.com/apache/seatunnel/pull/11493#issuecomment-6622991697
Re-checked the four points from my earlier review against head `ff53cb15bf52a9f44d7e3c9b6ae0bc35292b12c9` (`792ba76575` + `ff53cb15bf`), based on the self-review posted for this head: **PR11493-F1 (STRING → JSON source change with no opt-out)** — The self-review describes a `handle_json_as_string` source option as the explicit path back to the old `STRING` behavior, which is the shape I was asking for. Remaining asks: (a) confirm the option is honoured by all three affected sources (Snowflake `VARIANT`, Doris `JSON`/`JSONB`, StarRocks `JSON`), and (b) make sure `docs/en/introduction/concepts/incompatible-changes.md` spells out the default change and names the option so users hitting this on upgrade know how to restore the previous behavior. **PR11493-F2 (JSON logical type is physically a plain String, no validation outside the StarRocks load boundary)** — I don't see anything in this round touching that. Please state whether you intend to add validation at the JSON type boundary (or at the native-JSON sinks), or whether the documented position in `docs/en/introduction/concepts/schema-feature.md` is that JSON is a semantic tag only and malformed text fails at the destination. Either is acceptable to me if it is written down. **PR11493-F3 (StarRocks native JSON + default CSV Stream Load format only failing at load time)** — The note that `StarRocksIT.java:352-363` now asserts a sink-construction diagnostic suggests this moved to fail-fast, which is what I asked for. The asserted message itself wasn't included, so please paste the exact diagnostic and confirm `docs/en/connectors/sink/StarRocks.md` tells users to switch the load format when the target table has JSON columns. **PR11493-F4 (Snowflake user-defined query skipping `PARSE_JSON`)** — The description of the generated-insert path in `SnowflakeDialect.java:66-87` emitting `INSERT INTO t (id, payload) SELECT :id, PARSE_JSON(:payload)` for schemas with JSON fields, with the insert call sites in `JdbcOutputFormatBuilder.java` going through the `TableSchema` overload, suggests the generated-SQL side is covered; I'll confirm that against the diff. My original point was about the user-supplied query path, which is not mentioned. Please confirm what happens there: either validate/warn at sink initialization when JSON fields are present with a custom query, or document in `docs/en/connectors/sink/Snowflake.md` that the user is responsible for wrapping JSON binds in `PARSE_JSON`. Once F2 and F4 are answered and the F1/F3 doc confirmations are in, I'm fine with this from my side. <!-- streview-comment:2273 --> -- 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]
