SEZ9 commented on issue #12968: URL: https://github.com/apache/seatunnel/issues/12968#issuecomment-6406767161
Thanks @SEPURI-SAI-KRISHNA, that answers my question fully. Having the `isNull(rowIndex)` guard as the first statement of the generics `convert` overload in both `StructConverter` and `MapConverter`, placed before the dereference that used to fail, is exactly what I wanted to confirm, and it is good that it matches the shape `ListConverter` and `LargeListConverter` already use so every container converter handles a null cell the same way. I also appreciate you calling out the one difference rather than overstating the consistency. Agreed that `MapConverter` does not need an `isEmpty(rowIndex)` branch: an empty map already falls out of the normal path because the reader yields no entries, and `testEmptyMapCellStaysAnEmptyMap` pins that so empty and null stay distinct. A 6 line production diff across the two files is the right size for this fix. Nothing further from me on this point. The remaining ask is just procedural: please keep the three `ArrowToSeatunnelRowReaderNullCellTest` cases and the EN and ZH `incompatible-changes.md` entries for the MAP behaviour change (null MAP cell now reads as `null` where it used to read as an empty map) in the PR as described, and leave the older `// todo check map` in `ArrowToSeatunnelRowReaderTest` untouched as you proposed so the PR stays scoped to this one problem. Once that is in place I am happy to move forward on the PR side. <!-- streview-comment:2035 --> -- 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]
