DanielLeens commented on PR #11493:
URL: https://github.com/apache/seatunnel/pull/11493#issuecomment-5379351698
# What Problem Does This PR Solve?
- User pain point: Native semi-structured columns in Snowflake (`VARIANT`),
Doris (`JSON`/`JSONB`), and StarRocks (`JSON`) were flattened to plain `STRING`
in SeaTunnel's schema model, so pipelines lost the JSON structure end to end
and native-JSON sinks received escaped/quoted text instead of a structured
value.
- Fix approach: introduce a String-backed `SqlType.JSON` /
`BasicType.JSON_TYPE` logical type appended at the tail of the existing enum,
and thread it through the API core, JSON/CSV/Avro formats,
Calcite/Copy/JsonPath transforms, Spark translation, and the
Doris/StarRocks/Snowflake catalog, DDL-generation, and write paths, with
bilingual docs and an incompatible-change entry.
- One-sentence summary: the type-system design is genuinely sound, but the
current head (unchanged since 2026-08-14) still has two of its own new E2E
suites red in CI, and this pass independently confirms a real,
previously-unformalized *silent data loss* regression in the Qdrant connector
on top of the already-tracked open findings — this is not ready to merge.
**Housekeeping note up front, since I am reviewing my own PR again on a
completely unchanged head.** Head `8ddb8ff420f0` has not moved since my last
full review (submitted the same day, 2026-08-21 14:12:33 UTC). Rather than
mechanically re-deriving the entire 90-file diff a twelfth time and re-printing
an unchanged conclusion, this round (a) re-verifies live state, (b) spot-checks
two of the previously reported "still open" claims directly against source
rather than trusting my own prior text, and (c) independently traces one new
angle the prior rounds only mentioned in passing discussion but never
formalized as a numbered, evidenced Issue: what actually happens, today, when a
pre-existing first-party sink that has *not* been taught about `JSON` receives
a field of the new type. I am flagging this explicitly as a deviation from a
from-scratch re-derivation, with the justification above, per this project's
review-honesty requirements.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Live state re-verified just now, independent of the JSON context
snapshot:**
- `headRefOid`: `8ddb8ff420f0ee3e30f7da7f4321d1e6e1f153df` — unchanged since
2026-08-14.
- `isDraft`: `true` — still correctly marked Draft.
- `mergeStateStatus`: `BLOCKED`, `mergeable`: `MERGEABLE`.
- CI `Build` check: `FAILURE`, `COMPLETED` — same as reported in the last
full review; no new run has landed for this head.
- Comment/review counts on the PR are unchanged since my last post (5 issue
comments, 10 reviews) — nothing has been posted between my last review and this
one, so there is no unanswered reply to fold in.
**Independent spot-verification of two "still open" carryover claims**
(re-read from source on the current worktree, not re-asserted from my own prior
text):
1.
`seatunnel-formats/seatunnel-format-text/src/main/java/org/apache/seatunnel/format/text/TextSerializationSchema.java`
— confirmed no `case JSON:` in the `SqlType` switch (cases present: DOUBLE,
FLOAT, INT, BOOLEAN, TINYINT, SMALLINT, BIGINT, DECIMAL, STRING, DATE, TIME,
TIMESTAMP, TIMESTAMP_TZ, NULL, BYTES, ARRAY, MAP, ROW — no JSON). A JSON-typed
field reaching this path falls to whatever the `default:` branch does. Still
true.
2.
`seatunnel-connectors-v2/connector-doris/src/test/java/org/apache/seatunnel/connectors/doris/datatype/DorisTypeConvertorV1Test.java`
and `DorisTypeConvertorV2Test.java` — confirmed via `git diff
<merge-base>..HEAD -- <these files>` that both drop
`Assertions.assertEquals(MAX_STRING_LENGTH, column.getColumnLength())` in the
same hunk that changes the expected type from `STRING_TYPE` to `JSON_TYPE`,
with no replacement assertion on what `getColumnLength()` now returns for a
JSON column. Still true.
**New, independently-verified finding this round: the Qdrant sink silently
drops JSON-typed fields instead of erroring or storing them.**
I traced the actual field-conversion code, which the prior rounds had only
referenced by name in discussion, not walked through and evidenced as a formal
Issue:
```java
// QdrantBatchWriter.java (unchanged behavior, exposed by this PR's new
SqlType.JSON)
private Points.PointStruct buildPoint(SeaTunnelRow element) {
...
JsonWithInt.Value payloadValue = buildPayload(fieldType, value);
if (payloadValue != null) {
point.putPayload(fieldName, payloadValue);
continue;
}
Points.Vector vector = buildVector(fieldType, value);
if (vector != null) {
namedVectors.putVectors(fieldName, vector);
}
// if both are null, the field is silently omitted from the point
entirely
}
public static JsonWithInt.Value buildPayload(SeaTunnelDataType<?> fieldType,
Object value) {
switch (fieldType.getSqlType()) {
case SMALLINT: case INT: case BIGINT: ...
case STRING: case DATE: return ValueFactory.value(value.toString());
case BOOLEAN: return
ValueFactory.value(Boolean.parseBoolean(value.toString()));
default: return null; // <-- JSON falls here
}
}
public static Points.Vector buildVector(SeaTunnelDataType<?> fieldType,
Object value) {
switch (fieldType.getSqlType()) {
case FLOAT_VECTOR: case FLOAT16_VECTOR: case BFLOAT16_VECTOR: case
BINARY_VECTOR: ...
default: return null; // <-- JSON falls here too
}
}
```
Before this PR, a Doris/StarRocks/Snowflake JSON-bearing source column was
reported as `STRING`, so `buildPayload()` matched `case STRING:` and the value
was written to the point's payload as a plain string. After this PR, the same
source column is reported as `JSON`, which matches neither switch's explicit
cases, both helpers return `null`, and `buildPoint()` treats "both null" as
"not a payload field, not a vector field" — the field is dropped from the
`PointStruct` entirely, with **no exception, no log line, no metric**. A
previously-working Doris/StarRocks/Snowflake-source-to-Qdrant-sink pipeline
that includes a native-JSON column will, after this upgrade, keep running
successfully and silently write points that are missing that field, forever,
with no operator-visible signal that anything changed.
This is strictly worse than the already-documented Doris CSV/text gap (Issue
3 below), which at least throws. Contrast with `connector-selectdb-cloud`'s
`SeaTunnelRowConverter.convert()`, which I also independently checked and which
has the same kind of exhaustive switch — but its `default:` branch throws
`SelectDBConnectorException(UNSUPPORTED_DATA_TYPE, ...)`, so a
Doris-source-to-SelectDB-Cloud-sink pipeline hard-fails loudly instead of
silently losing data. Both connectors are outside this PR's own file list
(neither appears in the 90-file diff), which is exactly the risk category this
kind of "append an enum constant that other exhaustive switches don't know
about" change always carries, and it is why I don't think the incompatibility
surface of this PR is fully mapped yet.
Runtime path (JSON-typed field reaching an un-updated first-party sink):
```text
native source column (Doris JSON|JSONB / StarRocks JSON / Snowflake VARIANT)
-> catalog + type converter yield BasicType.JSON_TYPE (this PR)
-> value carried as JSON text; physical class is String.class
-> sink that has NOT been taught about JSON in this PR's diff:
connector-selectdb-cloud -> SeaTunnelRowConverter.convert() default:
throws UNSUPPORTED_DATA_TYPE (fail loud, at least)
connector-qdrant ->
QdrantBatchWriter.buildPayload()/buildVector() default: return null -> field
silently omitted from the point (fail silent, data loss)
seatunnel-format-text -> TextSerializationSchema, no case JSON
(Issue 3 below, first-party, Doris's own default CSV path)
```
## 1.2 Compatibility Impact
Partially incompatible, intentionally, and documented bilingually — the
mechanical properties I previously verified still hold on this unchanged head
(`SqlType.JSON` tail-appended, no ordinal shift, `BasicType.serialVersionUID`
unchanged, checkpoint/savepoint restore unaffected by the type addition itself,
old third-party `JdbcDialect` SPI implementations compile unchanged). What
remains genuinely incomplete is the documented blast radius of the schema
remap: this round adds one more concretely-verified, unlisted, first-party
casualty (Qdrant, silent field loss) to the two already tracked (Doris
CSV/text, unsupported-type throw; SelectDB Cloud, hard-fail per the discussion
thread, now independently confirmed by me above).
## 1.3 Performance / Side-Effect Analysis
No new hot-path allocation; unchanged from prior verification. JSON
parse/validate work is linear in payload size and confined to explicit
format/protocol boundaries.
## 1.4 Error Handling and Logging
See Issues 1–2 (CI) and Issue 3 (Qdrant silent drop, new this round) and
Issue 4 (Doris CSV/text, carryover). No sensitive-data logging introduced.
**Issue 1 (High, carryover, unchanged) —
`LocalFileIT.testLocalFileCsv`/`testLocalFileJson` both fail with a zero-row
round trip at the current head**
- Location:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-file-local-e2e/src/test/java/org/apache/seatunnel/e2e/connector/file/local/LocalFileIT.java:352,393`.
- Problem: confirmed by the completed fork CI run for the current,
still-unchanged head
(`https://github.com/DanielLeens/seatunnel/actions/runs/31789930614`). Both the
CSV and JSON LocalFile round-trip tests — this PR's own flagship new E2E
coverage — fail identically with a `MIN_ROW` assertion seeing 0 rows.
- Potential risk: the PR's documented flagship feature (LocalFile
round-tripping `json`/`array<json>`) may not actually work end to end.
- Best improvement: root-cause why the source produces zero rows for this
exact new fixture; do not merge unexplained.
- Severity: High. Raised by another reviewer: No.
**Issue 2 (High, carryover, unchanged) —
`DorisIT.assertJsonColumnsRoundTrip` JSON-null assumption is CI-confirmed
wrong, contradicting the corrected StarRocks sibling assertion**
- Location:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-doris-e2e/src/test/java/org/apache/seatunnel/e2e/connector/doris/DorisIT.java:283,295`;
contrast `connector-starrocks-e2e/.../StarRocksIT.java:526`.
- Problem: the same completed CI run shows `DorisIT.testDoris` failing 7
assertions, including the `sawJsonNull` expectation that assumes Doris
preserves an explicit JSON `null` distinctly from SQL `NULL`; the StarRocks
sibling test asserts the opposite, empirically-corrected behavior for the same
scenario.
- Potential risk: shipping with a known-failing, explicitly-required E2E
assertion for one of the PR's three named target connectors.
- Best improvement: verify actual Doris Stream Load JSON-null semantics and
align the test the same way StarRocks was already corrected.
- Severity: High. Raised by another reviewer: No.
**Issue 3 (High, new this round, independently verified) — Qdrant sink
silently drops JSON-typed fields with zero error, log, or metric**
- Location:
`seatunnel-connectors-v2/connector-qdrant/src/main/java/org/apache/seatunnel/connectors/seatunnel/qdrant/sink/QdrantBatchWriter.java`
(`buildPayload()` and `buildVector()` `default:` branches, `buildPoint()`'s
"both null -> omit field" handling).
- Problem: verified above — a JSON-typed field (new source-schema behavior
introduced by this PR) matches neither switch's cases in this pre-existing,
un-updated connector, both helpers return `null`, and the field is silently
omitted from the written `PointStruct`. No exception, no log, no metric.
- Potential risk: a previously-working Doris/StarRocks/Snowflake-to-Qdrant
pipeline keeps running after upgrade but silently loses a column's data on
every write, indefinitely, with no operator-visible signal. This is a
correctness regression, not merely a compatibility break.
- Best improvement: either (a) make `SqlType.JSON` explicitly rejected
(throw, matching the SelectDB Cloud pattern) in
`QdrantBatchWriter.buildPayload()`'s default branch instead of returning
`null`, since a "no match anywhere -> silently drop the field" behavior is
itself a latent bug for *any* future enum addition, not just this one, or (b)
add an explicit `case JSON:` that stores the JSON text as a payload string
(matching the pre-PR STRING behavior) if that is the intended migration path.
Either fix also strengthens the case for Issue 5's opt-out ask, since it
demonstrates the "no opt-out" blast radius is worse than previously documented.
- Severity: High — silent data loss on an existing, previously-correct
pipeline is more severe than a documented incompatibility with a clear error
message. Raised by another reviewer: No (this specific code-level trace and
default-branch analysis is new this round; the Qdrant connector was named in
passing in prior discussion-comment threads on this PR, but not previously
walked through in a numbered, evidenced review Issue).
**Issue 4 (High, carryover, unchanged) — STRING→JSON source-schema remap has
no opt-out, and the Doris CSV/text gap remains the concrete, reproducible
instance**
- Location: `docs/en/introduction/concepts/incompatible-changes.md:27-32`;
`seatunnel-formats/seatunnel-format-text/.../TextSerializationSchema.java` (no
`case JSON:`, re-confirmed above); `docs/en/connectors/sink/Doris.md:184`.
- Problem: a repository-wide grep still returns zero matches for any
`json.type.mapping`-style opt-out. The Doris CSV/text sink path — a
previously-working, zero-config pipeline — still throws a generic
unsupported-type error with no pointer to the documented `CAST(... AS VARCHAR)`
workaround, and the Doris sink docs still advertise CSV support without the
JSON caveat. Issue 3 above shows the same class of gap can manifest even more
dangerously (silently, not loudly) in a different first-party connector.
- Best improvement: add the missing `case JSON:` pass-through in
`seatunnel-format-text` (closes the concrete first-party regression) and/or add
a connector-level opt-out defaulting to legacy `STRING`; correct the Doris sink
docs either way.
- Severity: High. Raised by another reviewer: No (carryover from my own
prior round).
**Issue 5 (Low, blocking by project test-strictness policy, re-verified this
round) — Three type-converter tests dropped a `getColumnLength()` assertion
with no replacement**
- Location: `DorisTypeConvertorV1Test.java:307-308`,
`DorisTypeConvertorV2Test.java` (equivalent hunk),
`StarRocksTypeConverterTest.java` (equivalent hunk).
- Problem: re-confirmed via direct diff inspection this round (see 1.1) —
the `STRING_TYPE`→`JSON_TYPE` expectation change is correct, but the
accompanying `getColumnLength()` assertion is deleted with no replacement, so
nothing pins the intended contract for a JSON column's length going forward.
- Best improvement: add back an explicit assertion (e.g.
`assertNull(column.getColumnLength())` if that's the intended contract).
- Severity: Low, but blocking per this project's policy on unreplaced
reductions in test strictness. Raised by another reviewer: No.
**Issue 6 (Medium, carryover) — StarRocks CSV/JSON format rejection fires at
flush time, not at sink initialization**
- Location:
`seatunnel-connectors-v2/connector-starrocks/.../serialize/StarRocksBaseSerializer.java:77-80`.
- Severity: Medium. Raised by another reviewer: Yes (@SEZ9, Issue 3 of their
2026-08-13 review).
**Issue 7 (Medium, carryover) — Snowflake sink with a user-defined `query`
silently skips `PARSE_JSON` wrapping**
- Location:
`seatunnel-connectors-v2/connector-jdbc/.../dialect/snowflake/SnowflakeDialect.java:50-101`.
- Severity: Medium. Raised by another reviewer: Yes (@SEZ9, Issue 4 of their
2026-08-13 review).
**Issue 8 (Low, carryover) — Nested SQL-null/JSON-null distinction lost
inside `array<json>`**
- Location:
`seatunnel-formats/seatunnel-format-json/.../RowToJsonConverters.java:283-291`.
Severity: Low.
**Issue 9 (Low/Medium, carryover) — `starrocks.config.format` legacy shim
silently overrides `LOAD_FORMAT` with an unguarded `valueOf`**
- Location:
`seatunnel-connectors-v2/connector-starrocks/.../config/SinkConfig.java:97-103`.
Severity: Low/Medium.
## 2. Code Quality Assessment
### 2.1 Coding Standards
Unchanged from prior verification: title format correct, non-trivial new
methods generally carry Javadoc. `StarRocksCatalog.getNativeColumnTypes` still
lacks a comment explaining why it exists (Doris/StarRocks JSON columns report
as `VARCHAR` over the MySQL wire protocol) — Low/Medium per this project's
doc-comment standard.
### 2.2 Test Coverage and Test Stability
**Mandatory stability rating: Risk present.** Two CI-confirmed, reproducible
reasons (Issues 1–2), unchanged since the last round. No `Thread.sleep`-based
waits or floating-point-without-delta assertions found in the JSON-specific
test code.
### 2.3 Documentation Updates
`docs/en` and `docs/zh` remain updated in lockstep with a structured
incompatible-changes entry. Gap: the Doris CSV/text restriction and the Qdrant
silent-drop behavior are not reflected anywhere in the incompatible-changes doc
(Issues 3–4).
# 3. Architectural Soundness
## 3.1 Elegance of the Solution — Long-term solution
A String-backed logical type with opt-in, connector-native protocol mapping
remains the right boundary. What this round adds is evidence that the "surfaces
that have not opted in fail loudly" assumption (stated in my prior round's 3.3)
does not hold universally — Qdrant is a first-party counterexample that fails
silently instead.
## 3.2 Maintainability
Unchanged: type mapping, value validation, and protocol-specific
serialization stay cleanly separated; `JdbcDialect` default-method delegation
keeps the SPI contract stable.
## 3.3 Extensibility
Unchanged assessment, with one correction from this round: I previously
stated that surfaces without JSON support "fail loudly rather than silently
mis-stringifying," citing `seatunnel-format-text` as the one exception. That
statement was incomplete — Qdrant's field-level `null`-means-"no match, skip
this field" pattern is a second, and worse, exception, and it is a silent
*drop*, not a mis-stringify.
## 3.4 Historical-Version Compatibility
Unchanged from prior verification: `SqlType.JSON` is tail-appended, enum
serialization is by name, no per-`SqlType` serialization switch in the
engine/Flink translation path, existing checkpoint/savepoint state restores
unchanged after upgrade, old third-party `JdbcDialect` SPI implementations
compile unchanged.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|-------|----------|----------|
| 1 | `LocalFileIT` CSV/JSON round trip both fail with 0 rows — CI-confirmed
| LocalFileIT.java:352,393 | High |
| 2 | `DorisIT.assertJsonColumnsRoundTrip` JSON-null assumption CI-confirmed
wrong | DorisIT.java:283,295 | High |
| 3 | Qdrant sink silently drops JSON-typed fields, zero error/log/metric
(new this round) | QdrantBatchWriter.java (`buildPayload`/`buildVector`) | High
|
| 4 | STRING→JSON remap has no opt-out; Doris CSV/text is a demonstrated
zero-config regression | TextSerializationSchema.java; Doris.md:184 | High |
| 5 | `getColumnLength()` assertions dropped without replacement in three
converter tests | DorisTypeConvertorV1Test.java:307-308 + siblings | Low
(blocking by policy) |
| 6 | StarRocks CSV/JSON format rejection fires at flush, not sink init |
StarRocksBaseSerializer.java:77-80 | Medium |
| 7 | Snowflake sink with user-defined `query` skips `PARSE_JSON` |
SnowflakeDialect.java:50-101 | Medium |
| 8 | Nested `array<json>` SQL-null/JSON-null distinction lost |
RowToJsonConverters.java:283-291 | Low |
| 9 | `starrocks.config.format` shim silently overrides `LOAD_FORMAT` |
SinkConfig.java:97-103 | Low/Medium |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
**1. Blockers — must be fixed**
1. Issue 1 (High) — root-cause and fix the `LocalFileIT` zero-row failure.
2. Issue 2 (High) — align `DorisIT`'s JSON-null assumption with actual Doris
Stream Load behavior.
3. Issue 3 (High, new) — stop the Qdrant sink from silently dropping
JSON-typed fields; fail loud at minimum, or add real support.
4. Issue 4 (High) — close the Doris CSV/text gap and correct the Doris sink
docs.
5. Issue 5 (Low, policy-blocking) — restore the dropped `getColumnLength()`
assertions.
**2. Recommended fixes — non-blocking**
1. Issue 6 — move the StarRocks CSV/JSON format check to sink initialization.
2. Issue 7 — fail fast or warn for Snowflake + user-defined `query` + JSON
field.
3. Issues 8–9 — document/guard the remaining low-severity gaps.
**Overall assessment.** The type-system design remains sound and every
mechanical compatibility property I have independently re-verified across
multiple rounds still holds. What this round changes is the shape of the
evidence: the CI failures are unchanged and still block merge on their own, and
the "no opt-out for the schema remap" objection from the discussion thread is
now backed by a second, independently-traced, code-level example (Qdrant) that
is strictly worse than the ones already documented, because it fails silently
rather than loudly. As the PR author, this is not an independent approval — a
write-capable maintainer must still perform the final review — but from a
source-correctness standpoint this PR should not move out of Draft until Issues
1–5 are resolved and a completed CI run on the fixed head is green.
--
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]
