[FIX] fix pr review suggestions

Author: carpecodeum

parquet-variant-compute/src/variant_array.rs

@@ -179,18 +179,21 @@ impl VariantArray {
     /// Note: Does not do deep validation of the [`Variant`], so it is up to the
     /// caller to ensure that the metadata and value were constructed correctly.
     pub fn value(&self, index: usize) -> Variant<'_, '_> {
-        match &self.shredding_state {
-            ShreddingState::Unshredded { value } => {
+        match (self.shredding_state.value_field(), self.shredding_state.typed_value_field()) {
+            (Some(value), None) => {
+                // Unshredded case
                 Variant::new(self.metadata.value(index), value.value(index))
             }
-            ShreddingState::PerfectlyShredded { typed_value, .. } => {
+            (None, Some(typed_value)) => {
+                // PerfectlyShredded case
                 if typed_value.is_null(index) {
                     Variant::Null
                 } else {
                     typed_value_to_variant(typed_value, index)
                 }
             }
-            ShreddingState::ImperfectlyShredded { value, typed_value } => {
+            (Some(value), Some(typed_value)) => {
+                // ImperfectlyShredded case
                 if typed_value.is_null(index) {
                     Variant::new(self.metadata.value(index), value.value(index))
                 } else {
@@ -256,7 +259,7 @@ impl VariantArray {
 /// additional fields), or NULL (`v:a` was an object containing only the single expected field `b`).
 ///
 /// Finally, `v.typed_value.a.typed_value.b.value` is either NULL (`v:a.b` was an integer) or else a
-/// variant value.
+/// variant value (which could be `Variant::Null`).
 #[derive(Debug)]
 pub struct ShreddedVariantFieldArray {
     /// Reference to the underlying StructArray
@@ -361,7 +364,10 @@ impl Array for ShreddedVariantFieldArray {
     }
 
     fn nulls(&self) -> Option<&NullBuffer> {
-        self.inner.nulls()
+        // According to the shredding spec, ShreddedVariantFieldArray should be 
+        // physically non-nullable - SQL NULL is inferred by both value and 
+        // typed_value being physically NULL
+        None
     }
 
     fn get_buffer_memory_size(&self) -> usize {
@@ -451,6 +457,8 @@ impl ShreddingState {
             ShreddingState::PartiallyShredded { metadata, .. } => metadata,
             ShreddingState::AllNull { metadata } => metadata,
         }
+
+        Ok(Self { value, typed_value })
     }
 
     /// Return a reference to the value field, if present
@@ -506,7 +514,7 @@ impl ShreddingState {
 ///
 /// TODO: move to arrow crate
 #[derive(Debug, Default, Clone)]
-pub struct StructArrayBuilder {
+pub(crate) struct StructArrayBuilder {
     fields: Vec<FieldRef>,
     arrays: Vec<ArrayRef>,
     nulls: Option<NullBuffer>,