lidavidm commented on PR #35298: URL: https://github.com/apache/arrow/pull/35298#issuecomment-1520922061
> > 3\. Preserve nullability of map keys field. I'm +0.1 on this. It's not supposed to be nullable. However I suppose it's possible for the child to have a null bit map, so maybe it's better to take care of that in the `ValidateFull` method? > > I suppose this is always non-nullable for map keys field, otherwise it indicates a bug on the map type before serialization. It's validated: https://github.com/apache/arrow/blob/5de56928e0fe43f02005552eee058de57ffb2682/cpp/src/arrow/array/array_nested.cc#L404-L406 That said, I don't actually see a check on the field's nullability - we should fix that. > Preserve metadata of key and value fields. I'm +0.1. Seems fine but I'm not sure about the use case. We should just do this (as Gang has done) for consistency. No reason to special case it just because there isn't an obvious use case. > Good catch! I missed that. My question: is there any chance to prevent creating invalid sub-field names of map type? It's not validated: https://github.com/apache/arrow/blob/5de56928e0fe43f02005552eee058de57ffb2682/cpp/src/arrow/type.cc#L532-L537 This is also a potential compatibility issue. Off the top of my head, I might prefer: (1) ensure Validate checks for these cases, and (2) starting a bucket of things to enforce for a future MetadataVersion bump. (A hard error may invalidate files that used to be readable, and silently fixing the names may cause issues with things expecting round trips.) Or we could treat this as a C++ implementation problem, and (1) start issuing a warning + make sure Validate checks this, (2) turn this into a hard error with a C++/Python-specific flag to allow reading invalid files. -- 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]
