[GitHub] [arrow-rs] alamb commented on a change in pull request #246: fix parquet max_definition for non-null structs

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #246:
URL: https://github.com/apache/arrow-rs/pull/246#discussion_r625137887



##########
File path: parquet/src/arrow/levels.rs
##########
@@ -97,10 +97,20 @@ impl LevelInfo {
     /// Compute nested levels of the Arrow array, recursing into lists and structs.
     ///
     /// Returns a list of `LevelInfo`, where each level is for nested primitive arrays.
+    ///
+    /// The parent struct's nullness is tracked, as it determines whether the child
+    /// max_deginition should be incremented.

Review comment:
       ```suggestion
       /// max_definition should be incremented.
   ```

##########
File path: parquet/src/arrow/arrow_writer.rs
##########
@@ -881,10 +927,14 @@ mod tests {
         let schema = Schema::new(vec![field_a.clone()]);
 
         // create data
+        // When the null buffer of the struct is created, this test fails.
+        // It appears that the nullness of the struct is ignored when the
+        // struct is read back.
+        // See https://github.com/apache/arrow-rs/issues/245
         let c = Int32Array::from(vec![1, 2, 3, 4, 5, 6]);
         let b_data = ArrayDataBuilder::new(field_b.data_type().clone())
             .len(6)
-            .null_bit_buffer(Buffer::from(vec![0b00100111]))
+            // .null_bit_buffer(Buffer::from(vec![0b00100111]))

Review comment:
       I think this is ok (given the test didn't pass previously anyways) so this isn't a regression

##########
File path: parquet/src/arrow/levels.rs
##########
@@ -195,11 +208,12 @@ impl LevelInfo {
                             child_offsets,
                             child_mask,
                             false,
+                            false, // always false
                             list_field.is_nullable(),
                         )]
                     }
                     DataType::List(_) | DataType::LargeList(_) | DataType::Struct(_) => {
-                        list_level.calculate_array_levels(&child_array, list_field)
+                        list_level.calculate_array_levels(&child_array, list_field, false)

Review comment:
       if the data type is `DataType::Struct(_)`, wouldn't `is_parent_struct` be true? I probably misunderstand




-- 
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.

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