SEZ9 commented on PR #12397:
URL: https://github.com/apache/seatunnel/pull/12397#issuecomment-6538406196
Thanks for the follow-up on the array edge cases. The typed-array allocation
and the null-vs-empty map handling now look coherent across the source,
transform and sink conversion paths, so I don't see a remaining source blocker.
A few items from my earlier pass still need confirmation or a small follow-up:
**Robustness – `reconvertArray` element-class guard
(`SeaTunnelRowConverter`)**
All three `reconvertArray` overloads fill a typed array allocated from the
declared element class. Could you confirm that a declared-vs-runtime element
mismatch is rejected with field/table/type context rather than surfacing as a
bare `ArrayStoreException`? A small unit test covering that mismatch would
settle it.
**Logic – temporal element types in `TypeConverterUtils.convert(Spark
ArrayType)`**
The fallback to `ArrayType.of(elementType)` still yields lossy
`ARRAY<BIGINT>` / `ARRAY<DECIMAL(20,6)>` for TIME / TIMESTAMP_TZ arrays. Since
this Spark→SeaTunnel path has no production caller in the translation layer,
I'm fine keeping it, but please either add a short comment stating the
limitation or restore the explicit mapping so the behaviour is intentional.
**Style – `SeaTunnelArrayType`**
- Please add class-level Javadoc explaining that it is an allocation helper;
the current name reads like a data type, so a utility-style name may be worth
considering.
- The comment on `runtimeClass` only mentions the parsed array-of-map case.
Please extend it to note that it also intentionally ignores
`ArrayType.getTypeClass()` because of the mis-declared `LocalTimeType[].class`
constants in the API, and point at the parser defect (`new
ArrayType<>(MapType.class, ...)`) so a future reader knows why the logical type
is the source of truth.
**Docs**
- `docs/en/introduction/concepts/incompatible-changes.md`: please spell out
the concrete failure mode — a Spark sink writer that iterates `Map[]` elements
without a null check will now NPE where it previously saw `{}` — and name the
affected component using the module-path convention of the sibling entries
instead of the prose "Spark translation, including Spark 2.4 and 3.3 jobs".
- `docs/en/engines/spark.md` (and the zh mirror): the new "Nested Arrays"
section still reads like release notes ("Previously ...", "This
array-conversion fix does not ..."). Please rewrite it as reference prose
describing current behaviour only; the history belongs in the
incompatible-changes entry.
Once those are in, I'm happy to take another look.
<!-- streview-comment:2161 -->
--
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]
