[GitHub] [arrow-rs] nevi-me commented on a change in pull request #270: Fix null struct and list roundtrip

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

nevi-me commented on a change in pull request #270:
URL: https://github.com/apache/arrow-rs/pull/270#discussion_r629401310



##########
File path: parquet/src/arrow/levels.rs
##########
@@ -315,80 +314,40 @@ impl LevelInfo {
         // we use 64-bit offsets to also accommodate large arrays
         array_offsets: Vec<i64>,
         array_mask: Vec<bool>,
-        is_list: bool,
-        is_parent_struct: bool,
-        is_nullable: bool,
+        level_type: LevelType,
     ) -> Self {
         let min_len = *(array_offsets.last().unwrap()) as usize;
         let mut definition = Vec::with_capacity(min_len);
         let mut repetition = Vec::with_capacity(min_len);
         let mut merged_array_mask = Vec::with_capacity(min_len);
 
-        // determine the total level increment based on data types
-        let max_definition = match is_list {
-            false => {
-                // first exception, start of a batch, and not list
-                if self.max_definition == 0 {
-                    1
-                } else if self.is_list {
-                    // second exception, always increment after a list
-                    self.max_definition + 1
-                } else if is_parent_struct && !self.is_nullable {
-                    // if the parent is a non-null struct, don't increment
-                    self.max_definition
-                } else {
-                    self.max_definition + is_nullable as i16
-                }
+        let max_definition = match (self.level_type, level_type) {
+            (LevelType::Root, LevelType::Struct(is_nullable)) => {
+                // If the struct is non-nullable, its def level doesn't increment
+                is_nullable as i16
             }
-            true => {
-                if is_parent_struct && !self.is_nullable {
-                    self.max_definition + is_nullable as i16
-                } else {
-                    self.max_definition + 1 + is_nullable as i16
-                }
+            (LevelType::Root, _) => 1,
+            (_, LevelType::Root) => {
+                unreachable!("Cannot have a root as a child")
+            }
+            (LevelType::List(_), _) => {
+                // Always add 1  (TDDO: document why)

Review comment:
       Missed that, I'll fix that in a few hours




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