DanielLeens commented on PR #11493:
URL: https://github.com/apache/seatunnel/pull/11493#issuecomment-5340818101
Re-checking my own PR for this round. Head is unchanged since my last full
review (`8ddb8ff420f0`, merged from `dev` on 2026-08-14; no commit has landed
since). Rather than re-post a duplicate 23-issue review against an identical
code version, this is a **re-verification pass**: I re-derived the runtime path
independently and re-confirmed the highest-severity findings directly against
the current source, plus a **fresh CI signal** that post-dates the last review.
# What Problem Does This PR Solve?
**User pain.** Snowflake `VARIANT`, Doris `JSON`/`JSONB` and StarRocks
`JSON` columns are flattened to `STRING` today, so structure is lost
end-to-end: a JSON-aware sink receives an escaped string literal instead of a
native JSON value, and a JSON column cannot be declared in a `schema {}` block
at all.
**Fix approach.** A String-backed logical type: `SqlType.JSON` appended to
the enum, `BasicType.JSON_TYPE = new BasicType<>(String.class, SqlType.JSON)`,
wired through schema declaration, `SeaTunnelRow` byte accounting, Spark
translation, the JSON/CSV/Avro formats, the Calcite/Copy/JsonPath transforms,
and mapped to native types in Snowflake (`PARSE_JSON` on generated
INSERT/UPDATE), Doris and StarRocks.
**One-sentence summary.** The design is right and the plumbing is broad, but
the change is not yet safe to merge: two of this PR's own new E2E suites are
red, and I independently re-confirmed three separate silent/incorrect-behaviour
defects by reading the current head's source directly.
---
# 1. Code Change Review
## 1.1 Core Logic Analysis
### Runtime path (schema declaration -> row processing -> sink)
```text
schema { fields { payload = json } }
-> SeaTunnelDataTypeConvertorUtil.deserializeSeaTunnelDataType()
-> SqlType.valueOf("JSON") // name-based
lookup, safe
-> switch(sqlType) { case JSON: return BasicType.JSON_TYPE; }
Catalog discovery (StarRocks/Doris/Snowflake) also resolves to
BasicType.JSON_TYPE.
Row processing: value is always a java.lang.String holding JSON text.
SeaTunnelRow.getBytesForValue case STRING/JSON -> ((String) v).length()
[verified below]
SeaTunnelRow.getBytesForArray case STRING/JSON -> iterate String[]
[verified below]
Transform dispatch: type-blind for Flink (SeaTunnelRow carried as opaque
TypeInformation.of(SeaTunnelRow.class)); Spark and the
Calcite/Copy/JsonPath
transforms explicitly switch on SqlType.
Sink dispatch: 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.
```
### Independently re-verified findings (read directly from `8ddb8ff420f0`,
not carried over from the description)
**A. `SqlType`/`BasicType` wiring is correctly additive.**
```java
// seatunnel-api/.../SqlType.java
BFLOAT16_VECTOR,
SPARSE_FLOAT_VECTOR,
ROW,
- MULTIPLE_ROW;
+ MULTIPLE_ROW,
+ JSON;
```
`JSON` is appended as the *last* enum constant. That matters because
`BasicType`/`SeaTunnelDataType` objects never enter checkpoint state directly
in this codebase (rows travel through `RecordSerializer#writeObject`, and
per-connector checkpoint state is opaque `byte[]`), so the only place an
ordinal shift would matter is anywhere that does `SqlType.values()[i]`,
`.ordinal()`, or `EnumMap<SqlType,...>` positional logic. Appending last means
no existing ordinal shifts. This is the correct way to extend the enum.
**B. `CalciteTypeConverter` silently downgrades `ARRAY<JSON>` to
`ARRAY<STRING>` — confirmed by direct read of the full file at the current
head:**
```java
// seatunnel-transforms-v2/.../calcite/type/CalciteTypeConverter.java
// forward direction (SeaTunnel -> Calcite): JSON collapses into VARCHAR,
same as STRING
case STRING:
case JSON:
return factory.createSqlType(SqlTypeName.VARCHAR);
// reverse direction (Calcite -> SeaTunnel): VARCHAR always comes back as
STRING_TYPE.
// There is no branch, anywhere in this file, that can produce JSON_TYPE.
case CHAR:
case VARCHAR:
return BasicType.STRING_TYPE;
...
// array-element reverse mapping has the same gap
private static ArrayType<?, ?> convertToArrayType(SeaTunnelDataType<?>
elementType) {
SqlType sqlType = elementType.getSqlType();
switch (sqlType) {
case STRING:
return ArrayType.STRING_ARRAY_TYPE;
...
default:
return ArrayType.STRING_ARRAY_TYPE; // <- JSON falls here too
}
}
```
Because the forward mapping merges `JSON` and `STRING` into the same Calcite
`VARCHAR`, Calcite has no way to tell them apart on the way back, and the
reverse mapping unconditionally resolves `VARCHAR` to `BasicType.STRING_TYPE`.
A field or array element declared `json`/`array<json>` that passes through
*any* Calcite-transform-computed expression (not just an explicit CAST)
silently becomes `STRING`/`ARRAY<STRING>` — no exception, no log line, no
metric. This directly contradicts the same PR's own `array<json>` support at
the schema-declaration layer.
**C. The Qdrant sink silently drops JSON-typed columns — confirmed by direct
read of `QdrantBatchWriter.java`:**
```java
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 simply never written -- no error, no
log
}
public static JsonWithInt.Value buildPayload(SeaTunnelDataType<?> fieldType,
Object value) {
switch (fieldType.getSqlType()) {
case SMALLINT: case INT: case BIGINT: ...
case STRING: case DATE: ...
case BOOLEAN: ...
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
}
}
```
Both switches have a bare `default: return null;` and neither has a `case
JSON:`. The caller treats a `null` from both helpers as "this field is neither
a payload nor a vector, skip it." A JSON column written to Qdrant is therefore
silently omitted from every upserted point — the job reports success while
quietly discarding a column. This connector was not touched by the PR diff at
all, which is exactly the risk pattern with adding a new enum constant to a
public `SqlType`: every un-updated `default:` branch across ~150+ dispatch
sites is a latent behaviour change, and this is one of the ones that fails
silently instead of loudly.
**D. Both CI failures reproduce on a fresh rerun, two days after my last
review — not flaky, not stale.**
The fork's `Build` workflow run (`31789930614`) was rerun at attempt 3,
`run_started_at=2026-08-18T01:42:42Z`, i.e. after my 2026-08-16 review and with
**no intervening commit**. The same two jobs failed again:
- `connector-file-local-it (8)`: `LocalFileIT.testLocalFileJson` — `Tests
run: 102, Failures: 6` — `AssertConnectorException: row num :0 fail rule:
AssertFieldRule(ruleType=MIN_ROW, ruleValue=1.0)`. The read side sees zero
rows, meaning the write-side commit for the new JSON E2E configs is failing
before the assertion even runs.
- `doris-connector-it (11)`: `DorisIT` — `Tests run: 28, Failures: 7` — the
specific, reproducible failure:
```
org.opentest4j.AssertionFailedError: expected: <true> but was: <1>
at DorisIT.assertJsonColumnsRoundTrip(DorisIT.java:283)
at DorisIT.checkAllTypeSinkData(DorisIT.java:245)
at DorisIT.testDoris(DorisIT.java:196)
```
A JSON boolean `true` written to the source is read back from the Doris
sink as the integer `1`. This directly falsifies the PR's central claim
(structure/type fidelity is preserved) for a scalar JSON value on one of the
three target connectors.
Both failures are deterministic (same assertion, same line, reproduced
across a full rerun), not intermittent — this is a real functional gap, not CI
noise, and syncing with `dev` would not fix either of them since both are
exercising this PR's own new code and its own new test fixtures.
## 1.2 Compatibility Impact
**Partially incompatible**, and the two halves need to be told apart:
- *Additive and safe*: `json` was not previously a valid schema keyword
(`SqlType.valueOf("JSON")` used to throw), so declaring a `json` field is pure
new surface. `JSON` is appended last to the enum, and I confirmed no `SqlType`
reference exists anywhere under `seatunnel-engine/**`; connector checkpoint
state is opaque `byte[]`, so no historical serialized state can be
reinterpreted by this change.
- *Breaking, no opt-out*: Snowflake `VARIANT`, Doris `JSON`/`JSONB` and
StarRocks `JSON` **source** columns unconditionally switch from `STRING` to
`JSON` in the type converters. Any existing pipeline reading one of those
columns into a sink that has no `case JSON:` (which, per the findings above,
includes connectors this PR never touched, such as Qdrant) will change
behaviour on upgrade with no configuration escape — only a manual Calcite
`CAST(... AS VARCHAR)` workaround, which requires editing every affected job.
## 1.3 Performance / Side-Effect Analysis
No memory regression: a JSON value is a `java.lang.String`, identical in
cost to the `STRING` it replaces, and
`SeaTunnelRow.getBytesForValue`/`getBytesForArray` account for it exactly like
`STRING`. The added cost is CPU-only and confined to the paths that now parse
structure (`RowToJsonConverters`/`StarRocksJsonSerializer` calling `readTree()`
per row), which is inherent to the feature. No new locking, retries, or
resource-release concerns in the diff itself.
## 1.4 Error Handling and Logging
The dominant pattern across the ~150+ dispatch sites this change touches by
extension (adding an enum constant) is the safe one: an un-updated `switch`
throws with the type name rendered. The problem is the sites that don't follow
that pattern — items B and C above are exactly that: `default: return ...;`
with no error, no log, no metric, so a user has no signal that data was
silently reshaped or dropped. That is a materially worse failure mode than a
loud exception, and both are inside this PR's actual review surface (a
transform it advertises as JSON-aware, and connector code affected because the
source side of this PR now emits `JSON` for columns that used to be `STRING`).
**Issue 1 (High, blocking, CI red):**
`LocalFileIT.testLocalFileJson`/`testLocalFileCsv` — the write-side commit for
the new local-file JSON E2E configs fails and the assert job reads zero rows.
Reproduced fresh on the 2026-08-18 rerun, 6/6 across all three containers.
Location:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-file-local-e2e/src/test/resources/{csv,json}/*json_type*.conf`.
**Issue 2 (High, blocking, CI red):** `DorisIT.assertJsonColumnsRoundTrip` —
JSON boolean `true` round-trips through Doris as `1`. Reproduced fresh on the
2026-08-18 rerun. Location:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-doris-e2e/src/test/java/org/apache/seatunnel/e2e/connector/doris/DorisIT.java:283`.
**Issue 3 (High):** `CalciteTypeConverter` silently rewrites
`ARRAY<JSON>`/`JSON`-typed Calcite-computed expressions to
`ARRAY<STRING>`/`STRING`, with no error path at all. Location:
`seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/calcite/type/CalciteTypeConverter.java`
(`toSeaTunnelType`, `convertToArrayType`).
**Issue 4 (High):** the Qdrant sink silently discards any JSON-typed column
— no exception, no log, no metric; the job reports success while quietly
dropping data. Location:
`seatunnel-connectors-v2/connector-qdrant/src/main/java/org/apache/seatunnel/connectors/seatunnel/qdrant/sink/QdrantBatchWriter.java`
(`buildPayload`, `buildVector`).
**Issue 5 (High):** the source-side `STRING`->`JSON` remap for
Snowflake/Doris/StarRocks has no opt-out, and (per Issue 4) the actual blast
radius is wider than the connectors named in
`docs/en/introduction/concepts/incompatible-changes.md` — Qdrant is one
concrete example of a sink that silently mishandles the new type without ever
being mentioned as affected. Location:
`docs/en/introduction/concepts/incompatible-changes.md`.
I am not re-deriving the remaining findings from the 2026-08-16 review
(Parquet `ClassCastException`, SelectDB Cloud hard-fail, format-text/CSV gaps,
Zeta SQL CAST/COALESCE gaps, Assert self-confirming JSON comparison, FakeSource
`json` keyword gap, test-strictness regression in three converter tests, and
the documentation/nullability gaps) in this pass — the head is byte-identical
to what that review already covers in full detail, numbered Issue 1 through
Issue 23, and none of it has been addressed by a new commit. I am carrying that
full list forward unchanged rather than re-numbering or re-arguing it here.
---
# 2. Code Quality Assessment
## 2.1 Coding Standards
License headers, import style and formatting are consistent with project
convention across the files I read this round. New public/complex methods carry
Javadoc.
## 2.2 Test Coverage and Test Stability
**CI is red at this head**, and — importantly for this round — it is red on
a *fresh* rerun (`run_attempt=3`, started 2026-08-18T01:42:42Z, two days after
my last review, no intervening commit), not a stale signal from the earlier
2026-08-15 run. That rules out "it was just a transient CI blip that already
recovered."
**Stability rating: Stable (structurally), with two deterministic functional
failures.** I looked specifically for flaky-test anti-patterns (hard
`Thread.sleep` waits, order-dependence, nondeterministic assertions, unreleased
resources) in `LocalFileIT` and `DorisIT` and did not find that pattern here —
both failures are the *same* assertion, at the *same* line, on *every*
container, across *two separate CI runs three days apart*. That is the
signature of a real functional regression, not flakiness, and it should not be
treated as retry-and-hope; per the earlier review, the fix belongs in the
write-path configuration (Issue 1) and the boolean serialization path (Issue
2), not in loosening the assertions.
Coverage is broad on paper (new unit tests across the API convertor,
Doris/StarRocks/Snowflake type converters, Arrow, CSV/Avro/JSON formats,
Calcite, Copy, JsonPath; new E2E for Kafka, StarRocks, local file, and three
transforms), but two of the four new local-file E2E configs currently assert
nothing (Issue 1), so the real end-to-end proof is narrower than the file count
suggests. `KafkaIT.testJsonLogicalTypeFormat` remains the strongest end-to-end
evidence — it decodes an actual Kafka record and passes.
## 2.3 Documentation Updates
`docs/en` and `docs/zh` are both updated in parity for `Doris.md`,
`Snowflake.md`, `StarRocks.md` (source and sink), `schema-feature.md`, and
`incompatible-changes.md` — confirmed by the file list, matching pairs on both
sides of the diff.
---
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Long-term solution, and the right one.** A String-backed logical type
layered onto `SqlType` is the minimal design that preserves JSON semantics
without a new physical carrier, a new serializer, or a checkpoint-format change
— every un-updated path keeps treating the value as text instead of crashing
outright in most cases. A dedicated `JsonType` with a structured payload would
have been far more invasive and would have broken checkpoint compatibility; not
choosing it was correct.
## 3.2 Maintainability
The structural cost of this approach is real and now visible: adding a
`SqlType` constant creates on the order of 150+ dispatch sites a compiler
cannot check, and this round's independent spot-check found two more
silent-failure sites (Calcite, Qdrant) beyond what the source-side remap was
originally scoped to touch. That is not a flaw in the approach — it is the tax
this kind of enum extension always pays — but it means the "sweep every switch
over `SqlType`" pass needs to be genuinely exhaustive before merge, including
sites the PR itself never planned to touch, because the source-side remap
changes what type formerly-`STRING` data now carries through those very sites.
## 3.3 Extensibility
Good in principle — the pattern generalises to future logical types and
`array<json>` composition already works at the declaration layer — but the
`array<T>` composition is not uniformly honoured downstream yet (Calcite
discards it, confirmed above), so "extensible" and "consistently wired" are not
yet the same claim for this PR.
## 3.4 Historical-Version Compatibility
Clean for serialized/checkpoint state, for the reasons in 1.2: no `SqlType`
reference in the engine, opaque per-connector checkpoint bytes, and the new
enum value appended last. The compatibility risk in this PR is not in
serialized state — it is entirely in the source-side `STRING`-to-`JSON` schema
remap having no opt-out, which changes the observed schema of an
already-running pipeline on upgrade.
---
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | New local-file JSON E2E configs: write-side commit fails, 0 rows read,
assertion verifies nothing (CI red, reproduced fresh 2026-08-18) |
`connector-file-local-e2e/src/test/resources/{csv,json}/*json_type*.conf` |
High |
| 2 | JSON scalar boolean does not round-trip on Doris: source `true` vs
sink `1` (CI red, reproduced fresh 2026-08-18) | `DorisIT.java:283` | High |
| 3 | `ARRAY<JSON>`/JSON expressions silently rewritten to
`ARRAY<STRING>`/`STRING` by the Calcite transform, no error path |
`CalciteTypeConverter.java` (`toSeaTunnelType`, `convertToArrayType`) | High |
| 4 | Qdrant sink silently drops JSON columns — no error, no log, no data |
`QdrantBatchWriter.java` (`buildPayload`, `buildVector`) | High |
| 5 | STRING-to-JSON source remap has no opt-out and the documented blast
radius is narrower than the actual one (Qdrant is an undocumented example) |
`incompatible-changes.md` | High |
| 6-23 | Carried forward unchanged from the 2026-08-16 review (Parquet
`ClassCastException`, SelectDB Cloud hard-fail, format-text/Doris-CSV gap, CSV
MAP/ROW quoting, Milvus fallthrough, JSON-null key omission, connector
support-matrix gaps, StarRocks `format` precedence scope creep, Snowflake
`PARSE_JSON` inconsistency, `TypeUtil.canConvert` gap, Zeta SQL CAST/COALESCE
gaps, Assert self-confirming JSON comparison, FakeSource `json` keyword gap,
missing error context, dropped `columnLength` test assertions, `DorisIT`
`Object`-vs-`String` comparison, duplicated case bodies, undocumented
null-handling asymmetry) | see 2026-08-16 review | High/Medium/Low, unchanged |
---
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
**Blockers — must be fixed:**
- Issues 1 and 2 (High) — CI is red on this PR's own new E2E tests,
reproduced deterministically on a fresh rerun two days after the previous
review. These are functional gaps, not flakiness, and must be fixed rather than
relaxed.
- Issue 3 (High) — `CalciteTypeConverter` silently downgrades JSON to STRING
in the one transform this PR advertises as JSON-aware, contradicting the
`array<json>` support the same PR adds at the schema layer.
- Issue 4 (High) — the Qdrant sink silently discards JSON columns with zero
signal to the operator. Silent data loss blocks merge regardless of severity
elsewhere.
- Issue 5 (High) — the no-opt-out source remap is only acceptable as a
documented breaking change once the documented blast radius is complete; it
currently is not (Qdrant, SelectDB Cloud, and others are not listed).
- The remaining carried-forward High issues from the 2026-08-16 review
(Parquet `ClassCastException`, SelectDB Cloud hard-fail) apply unchanged, since
the code is unchanged.
**Recommended fixes — non-blocking:** the Medium/Low items carried forward
from the 2026-08-16 review (format-text/Doris-CSV mismatch, CSV MAP/ROW
quoting, Zeta SQL CAST/COALESCE gaps, error-message context, dropped test
assertions, documentation completeness). None of these alone block merge, but
several (format-text, Assert self-confirming comparison) compound the coverage
weakness in Issue 1.
**Overall assessment.** The core design is right — a String-backed logical
type appended last to `SqlType` is the minimal, checkpoint-safe way to add JSON
semantics, and the schema-declaration and checkpoint-compatibility stories are
genuinely clean. What is not yet safe is breadth of coverage: the two red E2E
suites are this PR's own new tests catching real gaps, and this round's
independent spot-check of Calcite and Qdrant confirms the "un-updated dispatch
site" risk is real and currently manifests as silent, undetectable behaviour
changes rather than loud failures in at least two places. None of this is a
design problem — every item is mechanically fixable — but it is not yet
demonstrably safe for an `seatunnel-api` change with this much fan-out.
**Upstream sync note (informational, not a blocker rationale):** current
head vs `dev` is `ahead_by=10`, `behind_by=28`, `diverged`. This is not why CI
is red — both failing jobs are exercising this PR's own new test fixtures and
new production code, so syncing `dev` would not resolve either one. I'd suggest
syncing before the next round of fixes purely to avoid the rebase growing
further, not as a troubleshooting step for the current failures.
--
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]
