SEPURI-SAI-KRISHNA commented on PR #12969:
URL: https://github.com/apache/seatunnel/pull/12969#issuecomment-6385886061
Thanks @DanielLeens, both points are handled.
**Sync done.** Rebased onto `dev` `9ab97e15f`, new head `8f2858bbc`. The PR
is 0 commits behind and reads mergeable again. The conflict was exactly where
you said: #12962 added an entry at the top of `## dev` in both files. I kept
both entries, with the arrow one above the JSON one. The diff of each file
against `dev` is now `+7/-0`, so nothing of #12962 was dropped.
`MapConverter.java`, `StructConverter.java` and the test are byte identical to
`6646025a`, so your code review still applies unchanged to this head.
Re-validated locally on the rebased head: `spotless:check` clean, and the
whole `connector-common` suite runs 14 tests with 0 failures, including the 3
new ones here. None of the 5 commits the rebase pulled in touch this module,
they are 3 doc commits, the JSON format change and an E2E guard.
**On Issue 1, I checked it and it does not reproduce.** You were careful to
flag that you had inferred the Arrow behaviour rather than run it, so I went
and measured it.
`StructVector.getObject` delegates to `NonNullableStructVector.getObject`,
which iterates the child field names and only puts an entry when the child's
own `getObject` is non null. In the Arrow 15.0.1 bytecode the put is guarded by
an `ifnull` branch that skips the key. So the map handed to `Collectors.toMap`
can never contain a null value, and the NPE you describe is not reachable
through that path.
I confirmed it on the real reader with two fixtures, a struct that is itself
non null in both:
| fixture | result |
| --- | --- |
| child `age` left unset | `profile={city=pune}`, no throw |
| child `age` written with an explicit `writeNull()` |
`profile={city=pune}`, no throw |
There is a real but milder effect underneath: the null child is dropped from
the map, so a consumer cannot tell "age is null" from "age was not in the
schema". That is pre existing, unrelated to this PR, and I did not widen the PR
for it. Happy to file it as its own issue if you think it is worth tracking.
CI is running again on the new head and I will report the result 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]
