DanielLeens commented on PR #11493:
URL: https://github.com/apache/seatunnel/pull/11493#issuecomment-5279227730
Thanks @SEZ9 for the extra pass. I re-checked each point against the current
head's source before responding, since there's no new commit since my last
review — this is a focused reply, not a new full review.
**Issue 1 (silent STRING->JSON remap, no opt-out) — relates to my own 1.2
finding, but I disagree that a config opt-out is required to merge.**
This is the same underlying fact I already flagged in section 1.2 of my last
review ("Partially incompatible... a real, if documented,
previously-working-pipeline regression") and
`docs/en/introduction/concepts/incompatible-changes.md` (the "Breaking Change:
Native semi-structured columns map to the JSON logical type" entry) confirms
there is indeed no config toggle, only the `CAST(payload AS VARCHAR)` migration
recipe — so the factual premise is correct. Where I'd push back is on treating
this as a High blocker that requires a `json.type.mapping` opt-out: this exact
same `dev` branch already carries a precedent for unconditional, opt-out-free
type remapping documented the same way — see the adjacent "Breaking Change:
Mapping of timezone-aware timestamp columns to TIMESTAMP_TZ type" entry in the
same file, which changed MySQL/PG/Oracle/SQL Server/Snowflake timestamp
mappings unconditionally with only a documented CAST-based migration, no opt-in
flag. If that bar was acceptable
for merge, requiring a new opt-out flag specifically for this PR isn't
consistent with project precedent, and I'd rather not block on it.
I also traced the checkpoint-restart claim specifically, since it's the part
of your Issue 1 I hadn't verified before:
`MultipleTableJobConfigParser.createAndPrepareSource` only takes the "reuse
checkpointed state" branch (`FactoryUtil.restoreAndPrepareSource(...,
checkpoint, ...)`) when `factory instanceof ChangeStreamTableSourceFactory`
(`FactoryUtil.java:139-149`) — that interface is CDC/change-stream specific
(e.g. MySQL-CDC). None of Doris/StarRocks/Snowflake implement it, so on both a
fresh submit and a restore-from-savepoint submit they go through the identical
`createAndPrepareSource(factory, ...)` -> `source.getProducedCatalogTables()`
path (`FactoryUtil.java:99-172`), which queries live catalog metadata every
time, not a schema cached in checkpoint state. So this isn't a distinct
checkpoint-corruption risk — a restored job sees exactly the same new JSON
schema a freshly-started job would, no more and no less. It's the same
already-documented compatibility fact, not a
new failure mode.
**Issue 2 (no client-side JSON validation before native sinks) — agree for
Snowflake, disagree for Doris.**
For Snowflake, this is accurate and a genuinely new point I hadn't
independently verified: `PARSE_JSON(:field)` binds go through plain `setString`
(`AbstractJdbcRowConverter.java:697-699`), so invalid content is only caught
server-side, per-batch, which is a real UX gap versus fail-fast client-side
validation.
For Doris, though, I checked
`connector-doris/.../serialize/SeaTunnelRowSerializer.java` and the JSON-format
branch (`LoadConstants.JSON`) constructs `new
JsonSerializationSchema(this.seaTunnelRowType, true)` — the same
`seatunnel-format-json` serializer whose
`RowToJsonConverters.createNotNullConverter` case I already verified re-parses
and rejects malformed/empty/trailing-token JSON via
`CommonError.jsonOperationError` before the row ever leaves the client (my
prior review, section 1.1). So for Doris's JSON Stream Load path specifically,
validation does happen client-side, before load — your "fails deep inside the
sink, per-batch, opaque" framing doesn't hold for that branch. It does still
apply to the Doris CSV branch, but that's the pre-existing gap I already logged
as my own Issue 2 (`seatunnel-format-text` has no `case JSON`) for a different
reason (unsupported type, not invalid content).
**Issue 3 (StarRocks CSV+JSON rejected at flush, not at sink init) — +1,
this is new and valid.**
I only looked at the CSV-rejection E2E test's stability (my Issue 4); I
hadn't examined *when* in the write lifecycle the rejection fires. I checked
`StarRocksBaseSerializer.convert()` and confirmed the `case JSON: throw ...` is
inside the per-value `convert()` method, called from row serialization during
writer flush — not from `open()`/`prepare()`. Your restart-loop concern for
streaming jobs is well-founded given that. Worth adding as a non-blocking
follow-up alongside my existing Issues.
**Issue 4 (Snowflake custom `query` bypasses PARSE_JSON wrapping) — +1, new
and valid, but already self-documented, so I'd keep it non-blocking.**
Matches my own read of `SnowflakeDialect`: `PARSE_JSON` wrapping lives in
`getInsertIntoStatement`/`getUpdateStatement`, which only apply to
SeaTunnel-generated statements, not a user-supplied `query`. The PR's own doc
note at `docs/en/connectors/sink/Snowflake.md:53` already discloses this, which
is the same documented-limitation pattern I accepted elsewhere in this PR (my
Issues 2/3/5) rather than a hard blocker.
**Net effect on my Merge Recommendation**: unchanged from my last review —
the actual blocker is still my Issue 1 (red CI on the Kafka/Doris lanes
exercising this PR's own JSON E2E coverage). I don't think Issue 1 here should
become an additional hard blocker given the TIMESTAMP_TZ precedent and the
checkpoint-restore correction above, but your Issue 2 (Snowflake half only) and
Issue 3 are legitimate additions to the non-blocking follow-up list together
with my Issues 2/3/5/6/7.
--
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]
