[GitHub] [arrow] mapleFU commented on a diff in pull request #35146: GH-29781: [C++][Parquet] Switch to use compliant nested types by default

[via GitHub]

mapleFU commented on code in PR #35146:
URL: https://github.com/apache/arrow/pull/35146#discussion_r1191488592


##########
cpp/src/parquet/properties.h:
##########
@@ -938,13 +937,13 @@ class PARQUET_EXPORT ArrowWriterProperties {
     /// list types (default "item"), will use "entries", as is specified in

Review Comment:
   Oops.
   
   ```c++
   MapType::MapType(std::shared_ptr<DataType> key_type, std::shared_ptr<Field> 
item_field,
                    bool keys_sorted)
       : MapType(::arrow::field("key", std::move(key_type), false), 
std::move(item_field),
                 keys_sorted) {}
   
   MapType::MapType(std::shared_ptr<Field> key_field, std::shared_ptr<Field> 
item_field,
                    bool keys_sorted)
       : MapType(
             ::arrow::field("entries",
                            struct_({std::move(key_field), 
std::move(item_field)}), false),
             keys_sorted) {}
   ```
   
   Found `"entries"` in `arrow/type.cc`, and spec is using "element". 
   
   And here maybe we're using "entries":
   
   ```c++
   /// In a field with Map type, the field has a child Struct field, which then
   /// has two children: key type and the second the value type. The names of 
the
   /// child fields may be respectively "entries", "key", and "value", but this 
is
   /// not enforced.
   ///
   /// Map
   /// ```text
   ///   - child[0] entries: Struct
   ///     - child[0] key: K
   ///     - child[1] value: V
   /// ```
   /// Neither the "entries" field nor the "key" field may be nullable.
   ```
   
   According to spec. Map should use "key_value". Seems it's incorrect? Am I 
right?



-- 
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: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org