Repository: jorgecarleitao/arrow2
Pull request #1248 (2022-09-12 22:26:22)

Add missing call to `try_push_valid` for nested avro deserialization
The validity len becomes out of sync compared to len (values[0].len) because there is no call to append valid after sucessfully deserializing a struct item. Add this functionality for `MutableSturctArray` similar to how it is handled for `MutableListArray`

jorgecarleitao: Hi @shaeqahmed . Thank you for the PR!

I am trying to understand why this is needed. My understanding is that `deserialize_value` only handles values, not nulls. The branching on the Union of `["null", type]` happens before this function is called, by `deserialize_item`, and there we add the validity accordingly.

I added #1250 on an attempt to argue that it works as is.
codecov[bot]: # [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) Report
> Merging [#1248](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (5ddb6ca) into [main](https://codecov.io/gh/jorgecarleitao/arrow2/commit/96df5e1ac8f48091e282db679d7687dd100cd889?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (96df5e1) will **decrease** coverage by `0.01%`.
> The diff coverage is `83.33%`.

```diff
@@            Coverage Diff             @@
##             main    #1248      +/-   ##
==========================================
- Coverage   83.15%   83.13%   -0.02%     
==========================================
  Files         359      359              
  Lines       38063    38069       +6     
==========================================
- Hits        31650    31649       -1     
- Misses       6413     6420       +7     
```


| [Impacted Files](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) | Coverage Δ | |
|---|---|---|
| [src/io/avro/read/nested.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2lvL2F2cm8vcmVhZC9uZXN0ZWQucnM=) | `62.21% <80.00%> (+0.41%)` | :arrow_up: |
| [src/io/avro/read/deserialize.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2lvL2F2cm8vcmVhZC9kZXNlcmlhbGl6ZS5ycw==) | `81.16% <100.00%> (+0.05%)` | :arrow_up: |
| [src/bitmap/utils/slice\_iterator.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC91dGlscy9zbGljZV9pdGVyYXRvci5ycw==) | `97.56% <0.00%> (-1.22%)` | :arrow_down: |
| [src/array/utf8/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L3V0ZjgvbW9kLnJz) | `82.71% <0.00%> (-0.93%)` | :arrow_down: |
| [src/io/ipc/read/file.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1248/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2lvL2lwYy9yZWFkL2ZpbGUucnM=) | `96.42% <0.00%> (-0.90%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao)

shaeqahmed: The logic for Structs's currently is to lazily init the validity array and pad it with true's followed by a false on the first appearance of a null. After this, whenever we deserialize an item, if it is the null variant of a union, we push_null(). However, if the struct is not null, meaning the struct is non-empty, we don't push a `true` to the validity after deserializing the fields. The equivalent for this is in line 136, where after deserializing all the values for a list, we make a call to  `try_push_valid` to push a true. I believe similar call is needed for MutableStructArray, and that's what I tried to add in this PR. I have a nested avro file that in the current logic triggers this issue, and fails to deserialize because the length of validity is too short.  

 I will try share it as an example, in addition to trying to create a minimal reproducible example for you. Please let me know if this makes sense.
shaeqahmed: Update: this PR is incorrect, as a naive call to `try_push_valid` here is not correct since that will run even if all the fields in the struct were null (which means the whole struct is null in parquet/arrow). I'm gonna put up another revision, but would appreciate a sanity check that a change is necessary here since this is my first time contributing to the Arrow format.
jorgecarleitao: You are right - this is a bug and the fix is this PR 👍 Thanks for the explanation.

If all elements are null, we still need to append the "true" to the StructArray. In arrow:
* the length of the struct's validity must be equal to the length of every field in the struct array.
* the validity of the struct is independent of the validity of its elements. In particular, there can be valid elements on a null slot and vice-versa. This is so that setting the validity of an array is `O(1)` (instead of a `O(N*D)`) operation (where `N` is length and `D` the max depth of the type).

You have a very good understanding of Arrow and this crate :)

I think this PR would just benefit from a test to confirm our hypothesis.
shaeqahmed: Awesome, that makes sense, working on the updated PR revision + test cases for this locally. Gonna put something up tonight. 👍 With this change, I can read in my deeply nested nullable structs to arrow2 now, but looks like the arrow2/parquet2 parquet writer is writing corrupt files. 

I see that in the code there are some explicitly unimplemented branches such as reading nulls from parquet, and the docs do mention lack of proper support for deeply nested parquet types (related issue:  https://github.com/jorgecarleitao/arrow2/issues/1222).

I think we should 1) better document what is currently supported and not for arrow2 parquet functionality, and 2) fix the above mentioned bug so that we don't at least write incorrect files. I've already opened an issue to track this here: https://github.com/jorgecarleitao/arrow2/issues/1249 so let's continue the discussion there. I'd appreciate if you could take a look. Thanks for the help

Patch:
diff --git a/src/io/avro/read/deserialize.rs b/src/io/avro/read/deserialize.rs
index 9fa9f15254..5f53565014 100644
--- a/src/io/avro/read/deserialize.rs
+++ b/src/io/avro/read/deserialize.rs
@@ -160,6 +160,7 @@ fn deserialize_value<'a>(
                 let values = array.mut_values(index);
                 block = deserialize_item(values, *is_nullable, &field.schema, block)?;
             }
+            array.try_push_valid()?;
         }
         _ => match data_type.to_physical_type() {
             PhysicalType::Boolean => {
diff --git a/src/io/avro/read/nested.rs b/src/io/avro/read/nested.rs
index 981b6894f9..649a702ff9 100644
--- a/src/io/avro/read/nested.rs
+++ b/src/io/avro/read/nested.rs
@@ -230,6 +230,14 @@ impl DynMutableStructArray {
         self.values[field].as_mut()
     }
 
+    #[inline]
+    pub fn try_push_valid(&mut self) -> Result<()> {
+        if let Some(validity) = &mut self.validity {
+            validity.push(true)
+        }
+        Ok(())
+    }
+
     #[inline]
     fn push_null(&mut self) {
         self.values.iter_mut().for_each(|x| x.push_null());


Test patch:
diff --git a/tests/it/io/avro/read.rs b/tests/it/io/avro/read.rs
index 5c301f7d1b..64ff34c7fe 100644
--- a/tests/it/io/avro/read.rs
+++ b/tests/it/io/avro/read.rs
@@ -47,7 +47,17 @@ pub(super) fn schema() -> (AvroSchema, Schema) {
                 "name": "",
                 "symbols" : ["SPADES", "HEARTS", "DIAMONDS", "CLUBS"]
             }},
-            {"name": "decimal", "type": {"type": "bytes", "logicalType": "decimal", "precision": 18, "scale": 5}}
+            {"name": "decimal", "type": {"type": "bytes", "logicalType": "decimal", "precision": 18, "scale": 5}},
+            {"name": "nullable_struct", "type": [
+                "null", {
+                    "type": "record",
+                    "name": "bla",
+                    "fields": [
+                        {"name": "e", "type": "double"}
+                    ]
+                }]
+                , "default": null
+            }
         ]
     }
 "#;
@@ -77,6 +87,11 @@ pub(super) fn schema() -> (AvroSchema, Schema) {
             false,
         ),
         Field::new("decimal", DataType::Decimal(18, 5), false),
+        Field::new(
+            "nullable_struct",
+            DataType::Struct(vec![Field::new("e", DataType::Float64, false)]),
+            true,
+        ),
     ]);
 
     (AvroSchema::parse_str(raw_schema).unwrap(), schema)
@@ -116,6 +131,12 @@ pub(super) fn data() -> Chunk<Box<dyn Array>> {
         PrimitiveArray::<i128>::from_slice([12345678i128, -12345678i128])
             .to(DataType::Decimal(18, 5))
             .boxed(),
+        StructArray::from_data(
+            DataType::Struct(vec![Field::new("e", DataType::Float64, false)]),
+            vec![Box::new(PrimitiveArray::<f64>::from_slice([1.0, 0.0]))],
+            Some([true, false].into()),
+        )
+        .boxed(),
     ];
 
     Chunk::try_new(columns).unwrap()
@@ -157,6 +178,13 @@ pub(super) fn write_avro(codec: Codec) -> std::result::Result<Vec<u8>, avro_rs::
         "duration",
         Value::Duration(Duration::new(Months::new(1), Days::new(1), Millis::new(1))),
     );
+    record.put(
+        "nullable_struct",
+        Value::Union(Box::new(Value::Record(vec![(
+            "e".to_string(),
+            Value::Double(1.0f64),
+        )]))),
+    );
     writer.append(record)?;
 
     let mut record = Record::new(writer.schema()).unwrap();
@@ -187,8 +215,9 @@ pub(super) fn write_avro(codec: Codec) -> std::result::Result<Vec<u8>, avro_rs::
             255u8, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 67, 158, 178,
         ])),
     );
+    record.put("nullable_struct", Value::Union(Box::new(Value::Null)));
     writer.append(record)?;
-    Ok(writer.into_inner().unwrap())
+    writer.into_inner()
 }
 
 pub(super) fn read_avro(