SEZ9 commented on PR #12397:
URL: https://github.com/apache/seatunnel/pull/12397#issuecomment-5923899302
Thanks for merging dev again. The PR-owned files are identical between
`7c5629e4` and `809a2d96` (the only delta is the unrelated
`incompatible-changes.md` entries that came in via the merge), so my earlier
conclusions carry over and I'm happy to move this to approval.
For completeness, the earlier points still stand as non-blocking suggestions
— feel free to address them here or in a follow-up:
- **F1 (`SeaTunnelRowConverter.java`, `reconvertArray` overloads):** the
typed array is allocated from the declared element class without an
element-class guard, so a declared-vs-runtime mismatch surfaces as a bare
`java.lang.ArrayStoreException` without field/table/type context. A guard or
catch-and-wrap that adds that context would make failures easier to diagnose.
- **F2 (`TypeConverterUtils.java`):** `convert(Spark ArrayType)` now returns
`ArrayType.of(elementType)` silently for temporal element types, yielding 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,
either an explicit rejection or a short comment documenting the lossy behaviour
would be fine.
- **F3 / F4 (`SeaTunnelArrayType.java`):** a class-level Javadoc, possibly a
utility-style name, and expanding the `runtimeClass` comment to mention both
reasons for ignoring `ArrayType.getTypeClass()` (the parsed array-of-map case
`new ArrayType<>(MapType.class, ...)` and the mis-declared
`LocalTimeType[].class` constants).
- **F5 (`incompatible-changes.md`):** null map elements inside arrays now
reach Spark sinks as `null` instead of `{}`. This aligns Spark with Zeta, but
calling out the concrete failure mode (a sink writer iterating `Map[]` elements
without a null check will now NPE) would help users.
- **F6 (`docs/en/engines/spark.md` + zh mirror):** the "Nested Arrays"
section mixes release-note / PR-scope wording ("Previously ...", "This
array-conversion fix does not ...") into a reference page; a plain description
of current behaviour, with the history kept in the incompatible-changes entry,
would read better.
- **F7 (`incompatible-changes.md`):** consider naming the affected component
with the module-path convention used by sibling entries rather than the prose
form.
Thanks for the careful work on this one.
<!-- streview-comment:1444 -->
--
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]
