DanielLeens commented on PR #11493:
URL: https://github.com/apache/seatunnel/pull/11493#issuecomment-6538235517
Pushed the promised follow-up. New head:
`925fb9c1fe0da5b6cd1b0a65b7fbcd036f22a16b` (two commits on top of
`58d1cf0d19d`: the `dev` merge `c73d493a963` and the review-fix commit
`925fb9c1fe0`).
**Dev sync.** Merged `origin/dev` @ `3ac2d1cec6a` (the branch was 298
commits behind; GitHub now reports the PR as mergeable). 8 files conflicted,
all "both sides added at the same place", resolved by keeping both sides:
- `docs/{en,zh}/introduction/concepts/schema-feature.md`: array element-type
list now reads `int string json boolean ... double map` and nested `array`
(PR's `json` + dev's `map`/nested `array`).
- `connector-doris/.../AbstractDorisTypeConverter.java`: kept the PR's
`DORIS_JSON`/`DORIS_JSONB -> JSON_TYPE` case next to dev's new
`DORIS_IPV4`/`DORIS_IPV6` cases.
- `connector-common/.../ArrowToSeatunnelRowReaderTest.java`: import block,
took dev's superset.
- `TestCopyIT.java`, `TestJsonPathTransformIT.java`,
`JsonRowDataSerDeSchemaTest.java`, `CopyFieldMultiCatalogTransformTest.java`:
the PR's JSON-type test methods and dev's `table_transform` / container-type /
timestamp-tz tests were appended at the same position; both sets are kept.
No `pom.xml` was touched, and `git diff origin/dev --name-status` shows no
deletions.
**Review items, mapped to the new head**
- **F1 (High, compatibility) - route taken: opt-out option, default keeps
the new JSON mapping.** New option `handle_json_as_string` (boolean, default
`false`) on the sources that infer their schema from the database:
- Doris source: `DorisSourceOptions.java:105`, applied in
`DorisSourceFactory.java:113-117` right after `catalog.getTable(...)`.
- Jdbc source (Snowflake `VARIANT`): `JdbcSourceOptions.java:189`, applied
in `JdbcSource.java:72-86` (runtime tables) and `JdbcSourceFactory.java:152`
(dry-run schema), via a new `JdbcSourceTable.withCatalogTable` copy.
- Both use `connector-common/.../LegacyJsonTypeUtil.java:52`
(`mapJsonColumnsToString`), which rewrites top-level `JSON` columns to `STRING`
and keeps PK/constraints/options/metadata; test `LegacyJsonTypeUtilTest`.
- StarRocks source: no option, on purpose.
`StarRocksSourceTableConfig.parseStarRocksSourceConfig` always builds the
schema from the user-declared `schema` (`ReadonlyConfigParser.parse` throws
without it), so there is no inferred mapping to opt out of: declaring the
column as `string` keeps the old behavior, `json` opts into the logical type.
Documented in `docs/en/connectors/source/StarRocks.md:24`. The StarRocks
mapping change only affects `StarRocksCatalog` / `StarRocksDataTypeConvertor`
and auto table creation.
- `docs/en/introduction/concepts/incompatible-changes.md:457` (zh `:382`)
now has an explicit **Opt-out** paragraph and spells out the Databend (STRING
DDL + warning), Easysearch (string type) and Qdrant (rejected) behavior. Option
rows added to `docs/en/connectors/source/Doris.md:84`, `Snowflake.md:76`,
`Jdbc.md:177` and the zh counterparts.
- **F2 (Medium, docs)**:
`docs/en/introduction/concepts/schema-feature.md:161` (zh `:161`) states the
contract: `json` guarantees the representation (JSON text, Java `null` = SQL
null, string `null` = JSON null), not validity; validity is enforced only at
the parsing boundaries (JSON format, StarRocks JSON Stream Load serializer,
Snowflake `PARSE_JSON` server-side); every other sink passes the text through,
so producing valid JSON is the source/transform's responsibility and rejecting
unstorable text is the native JSON sink's responsibility.
- **F3 (Medium, robustness)**: `StarRocksSink.java:71` calls
`validateJsonColumnsAgainstLoadFormat` (`:147`), which throws
`StarRocksConnectorException(UNSUPPORTED_DATA_TYPE)` in the sink constructor
(i.e. at job submission) when the Stream Load format is CSV and the schema has
`JSON` columns, naming the columns and pointing at `starrocks.config.format =
"JSON"` / `CAST(... AS VARCHAR)`. The CSV serializer rejection stays as defense
in depth. Doc: `docs/en/connectors/sink/StarRocks.md:243` (zh `:238`). Test
`StarRocksSinkTest` (CSV+JSON throws, JSON format accepted).
- **F4 (Medium, robustness)**: fail-fast route. New
`JdbcDialect.validateUserDefinedQuery` default no-op hook
(`JdbcDialect.java:971`), invoked from the `JdbcSink` constructor
(`JdbcSink.java:114`). `SnowflakeDialect.java:120-132` overrides it: with a
user-defined `query`, JSON fields in the schema and no `PARSE_JSON` in the
query text, the sink throws `JdbcConnectorException(ILLEGAL_ARGUMENT)` naming
the fields and telling the user to wrap the placeholder with `PARSE_JSON(?)` or
drop `query`. A query that already contains `PARSE_JSON` passes, so the
documented manual workaround keeps working. Doc:
`docs/en/connectors/sink/Snowflake.md:56` (zh `:56`). Test
`SnowflakeDialectTest`.
- **Issue 3 (High, Qdrant)**: `QdrantBatchWriter.java:171` adds an explicit
`case JSON` that throws `QdrantConnectorException(UNSUPPORTED_DATA_TYPE)`
instead of falling into the `default: return null` branch that dropped the
column. Test `QdrantBatchWriterTest` (JSON rejected, STRING payload unchanged).
- **Issues 4-5 (Databend/Easysearch)**: documentation only, in the
incompatible-changes entry above; no code change.
**Validation.** Per the project validation policy I only ran
`spotless:apply` + `spotless:check` locally on the touched modules
(`connector-common`, `connector-jdbc`, `connector-doris`,
`connector-starrocks`, `connector-qdrant`, plus the conflict-resolved modules);
both passed. Compile, unit tests and E2E are left to CI. The fork Build for
this head is queued:
https://github.com/DanielLeens/seatunnel/actions/runs/41163130794 . The E2E
outcome (Doris, StarRocks, Kafka, LocalFile, transforms; Snowflake stays
class-level disabled as before) will be read from that run once it completes, I
am not predicting it here.
--
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]
