SEZ9 commented on issue #12968: URL: https://github.com/apache/seatunnel/issues/12968#issuecomment-6392344498
Thanks @SEPURI-SAI-KRISHNA, that answers my question. The fixture you describe is exactly what was missing: a real `MapVector` populated through `UnionMapWriter` with a written entry on row 0, `setNull(1)` on row 1 and an empty `startMap()`/`endMap()` pair on row 2, serialized with `ArrowStreamWriter` and read back through the production byte array constructor of `ArrowToSeatunnelRowReader`. That exercises the real dispatch rather than a converter in isolation, and the three assertions in `ArrowToSeatunnelRowReaderNullCellTest` cover the behaviours I raised: - `testNullStructCellReadsAsNull` for the `StructConverter` NPE on a null ROW cell - `testNullMapCellReadsAsNull` for the `MapConverter` returning an empty map on a null MAP cell - `testEmptyMapCellStaysAnEmptyMap` to make sure a genuinely empty map is not collapsed to `null` Leaving the older `// todo check map` in `ArrowToSeatunnelRowReaderTest` untouched is fine with me; keeping the PR on one problem is the right call, and the populated MAP coverage now lives in the new class. Adding the EN and ZH `incompatible-changes.md` entries for the MAP change is also correct, since users that relied on a null MAP cell arriving as an empty map will see `null` now. One small thing: could you confirm the fix adds the validity bit check in the generic `convert` overload of both `StructConverter` and `MapConverter`, in the same shape as the three list converters, so all of the container converters handle null cells consistently? I'll take the rest of the review over to the PR. Thanks for the thorough write-up. <!-- streview-comment:2010 --> -- 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]
