Merge pull request #249 from chmp/fix/sliced-lists

Incorrect deserialization from sliced arrays

Changes.md

@@ -1,5 +1,19 @@
 # Change log
 
+## 0.12.2
+
+Bug fixes:
+
+- Fixed deserialization from sliced arrays ([#248](https://github.com/chmp/serde_arrow/issues/248)).
+  Note that the current solution requires up front work when constructing the array deserializers,
+  as described in the issue. The removal of the performance penalty is tracked in
+  ([#250](https://github.com/chmp/serde_arrow/issues/250))
+
+### Thanks
+
+- [@jkylling](https://github.com/jkylling) for reporting
+  ([#248](https://github.com/chmp/serde_arrow/issues/248)) and for discussing potential solutions
+
 ## 0.12.1
 
 New features

serde_arrow/src/internal/deserialization/array_deserializer.rs

@@ -296,7 +296,12 @@ impl<'a> ArrayDeserializer<'a> {
                     fields.push((field_meta.name, field_deserializer))
                 }
 
-                Ok(Self::Enum(EnumDeserializer::new(path, view.types, fields)))
+                Ok(Self::Enum(EnumDeserializer::new(
+                    path,
+                    view.types,
+                    view.offsets,
+                    fields,
+                )?))
             }
         }
     }

serde_arrow/src/internal/deserialization/enum_deserializer.rs

@@ -1,4 +1,4 @@
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 
 use serde::de::{DeserializeSeed, Deserializer, EnumAccess, Visitor};
 
@@ -20,15 +20,80 @@ impl<'a> EnumDeserializer<'a> {
     pub fn new(
         path: String,
         type_ids: &'a [i8],
-        variants: Vec<(String, ArrayDeserializer<'a>)>,
-    ) -> Self {
-        Self {
+        offsets: &'a [i32],
+        mut variants: Vec<(String, ArrayDeserializer<'a>)>,
+    ) -> Result<Self> {
+        let initial_offsets = verify_offsets(type_ids, offsets, variants.len())?;
+
+        for (type_id, initial_offset) in initial_offsets {
+            let Some((_, variant)) = variants.get_mut(type_id as usize) else {
+                fail!("Unexpected error: could not retrieve variant {type_id}");
+            };
+            variant.skip(initial_offset as usize)?;
+        }
+
+        Ok(Self {
             path,
             type_ids,
             variants,
             next: 0,
+        })
+    }
+}
+
+fn verify_offsets(type_ids: &[i8], offsets: &[i32], num_fields: usize) -> Result<HashMap<i8, i32>> {
+    if type_ids.len() != offsets.len() {
+        fail!("Offsets and type ids must have the same length")
+    }
+
+    for &type_id in type_ids {
+        if type_id as usize >= num_fields {
+            fail!(
+                concat!(
+                    "Invalid enum array:",
+                    "type id ({type_id}) larger the number of fields ({num_fields})",
+                ),
+                type_id = type_id,
+                num_fields = num_fields,
+            );
+        }
+    }
+
+    let mut last_offsets = HashMap::<i8, i32>::new();
+    let mut initial_offsets = HashMap::<i8, i32>::new();
+    for (idx, (&type_id, &offset)) in std::iter::zip(type_ids, offsets).enumerate() {
+        if offset < 0 {
+            fail!(
+                concat!(
+                    "Invalid offsets in enum array for item {idx}:",
+                    "negative offsets ({offset}) is not supports",
+                ),
+                idx = idx,
+                offset = offset,
+            );
+        }
+        if let Some(last_offset) = last_offsets.get(&type_id).copied() {
+            if offset.checked_sub(last_offset) != Some(1) {
+                fail!(
+                    concat!(
+                        "Invalid offsets in enum array for item {idx}:",
+                        "serde_arrow only supports consecutive offsets.",
+                        "Current offset for type {type_id}: {offset}, previous offset {last_offset}",
+                    ),
+                    idx = idx,
+                    type_id = type_id,
+                    offset = offset,
+                    last_offset = last_offset,
+                );
+            }
+            last_offsets.insert(type_id, offset);
+        } else {
+            initial_offsets.insert(type_id, offset);
+            last_offsets.insert(type_id, offset);
         }
     }
+
+    Ok(initial_offsets)
 }
 
 impl<'de> Context for EnumDeserializer<'de> {

serde_arrow/src/internal/deserialization/list_deserializer.rs

@@ -23,11 +23,12 @@ pub struct ListDeserializer<'a, O: Offset> {
 impl<'a, O: Offset> ListDeserializer<'a, O> {
     pub fn new(
         path: String,
-        item: ArrayDeserializer<'a>,
+        mut item: ArrayDeserializer<'a>,
         offsets: &'a [O],
         validity: Option<BitsWithOffset<'a>>,
     ) -> Result<Self> {
         check_supported_list_layout(validity, offsets)?;
+        item.skip(offsets[0].try_into_usize()?)?;
 
         Ok(Self {
             path,

serde_arrow/src/internal/deserialization/map_deserializer.rs

@@ -24,13 +24,17 @@ pub struct MapDeserializer<'a> {
 impl<'a> MapDeserializer<'a> {
     pub fn new(
         path: String,
-        key: ArrayDeserializer<'a>,
-        value: ArrayDeserializer<'a>,
+        mut key: ArrayDeserializer<'a>,
+        mut value: ArrayDeserializer<'a>,
         offsets: &'a [i32],
         validity: Option<BitsWithOffset<'a>>,
     ) -> Result<Self> {
         check_supported_list_layout(validity, offsets)?;
 
+        let initial_offset = usize::try_from(offsets[0])?;
+        key.skip(initial_offset)?;
+        value.skip(initial_offset)?;
+
         Ok(Self {
             path,
             key: Box::new(key),

serde_arrow/src/internal/deserialization/simple_deserializer.rs

@@ -1,4 +1,7 @@
-use serde::{de::Visitor, Deserializer};
+use serde::{
+    de::{IgnoredAny, Visitor},
+    Deserializer,
+};
 
 use crate::internal::{
     error::{fail, Context, Error, Result},
@@ -7,6 +10,13 @@ use crate::internal::{
 
 #[allow(unused)]
 pub trait SimpleDeserializer<'de>: Context + Sized {
+    fn skip(&mut self, n: usize) -> Result<()> {
+        for _ in 0..n {
+            self.deserialize_any(IgnoredAny)?;
+        }
+        Ok(())
+    }
+
     fn deserialize_any<V: Visitor<'de>>(&mut self, visitor: V) -> Result<V::Value> {
         fail!(in self, "Deserializer does not implement deserialize_any");
     }

serde_arrow/src/internal/deserialization/utils.rs

@@ -77,21 +77,44 @@ pub fn check_supported_list_layout<'a, O: Offset>(
     validity: Option<BitsWithOffset<'a>>,
     offsets: &'a [O],
 ) -> Result<()> {
-    let Some(validity) = validity else {
-        return Ok(());
-    };
-
     if offsets.is_empty() {
         fail!("Unsupported: list offsets must be non empty");
     }
 
     for i in 0..offsets.len().saturating_sub(1) {
         let curr = offsets[i].try_into_usize()?;
         let next = offsets[i + 1].try_into_usize()?;
-        if !bitset_is_set(&validity, i)? && (next - curr) != 0 {
-            fail!("Unsupported: lists with data in null values are currently not supported in deserialization");
+
+        if next < curr {
+            fail!("Unsupported: list offsets are assumed to be monotonically increasing");
+        }
+        if let Some(validity) = validity.as_ref() {
+            if !bitset_is_set(&validity, i)? && (next - curr) != 0 {
+                fail!("Unsupported: lists with data in null values are currently not supported in deserialization");
+            }
         }
     }
 
     Ok(())
 }
+
+#[test]
+fn test_check_supported_list_layout() {
+    use crate::internal::testing::assert_error_contains;
+
+    assert_error_contains(&check_supported_list_layout::<i32>(None, &[]), "non empty");
+    assert_error_contains(
+        &check_supported_list_layout::<i32>(None, &[0, 1, 0]),
+        "monotonically increasing",
+    );
+    assert_error_contains(
+        &check_supported_list_layout::<i32>(
+            Some(BitsWithOffset {
+                offset: 0,
+                data: &[0b_101],
+            }),
+            &[0, 5, 10, 15],
+        ),
+        "data in null values",
+    );
+}

serde_arrow/src/test/deserialization/list.rs

@@ -0,0 +1,40 @@
+use crate::internal::{
+    arrow::{ArrayView, FieldMeta, ListArrayView, PrimitiveArrayView},
+    deserialization::array_deserializer::ArrayDeserializer,
+    testing::assert_error_contains,
+};
+
+#[test]
+fn invalid_offsets() {
+    let reference = ListArrayView {
+        validity: None,
+        offsets: &[],
+        meta: FieldMeta {
+            name: String::from("element"),
+            nullable: false,
+            metadata: Default::default(),
+        },
+        element: Box::new(ArrayView::Int32(PrimitiveArrayView {
+            validity: None,
+            values: &[0, 1, 2, 3, 4, 5],
+        })),
+    };
+
+    let view = ArrayView::List(ListArrayView {
+        offsets: &[],
+        ..reference.clone()
+    });
+    assert_error_contains(
+        &ArrayDeserializer::new(String::from("foo"), None, view),
+        "non empty",
+    );
+
+    let view = ArrayView::List(ListArrayView {
+        offsets: &[0, 5, 2],
+        ..reference.clone()
+    });
+    assert_error_contains(
+        &ArrayDeserializer::new(String::from("foo"), None, view),
+        "monotonically increasing",
+    );
+}

serde_arrow/src/test/deserialization/mod.rs

@@ -0,0 +1,2 @@
+mod list;
+mod r#union;

serde_arrow/src/test/deserialization/union.rs

@@ -0,0 +1,57 @@
+use crate::internal::{
+    arrow::{ArrayView, DenseUnionArrayView, FieldMeta, PrimitiveArrayView},
+    deserialization::array_deserializer::ArrayDeserializer,
+    testing::assert_error_contains,
+};
+
+#[test]
+fn non_consecutive_offsets() {
+    let fields = vec![
+        (
+            0,
+            ArrayView::Int32(PrimitiveArrayView {
+                validity: None,
+                values: &[1, 2, 3, 4, 5, 6],
+            }),
+            FieldMeta {
+                name: String::from("foo"),
+                nullable: false,
+                metadata: Default::default(),
+            },
+        ),
+        (
+            1,
+            ArrayView::Int32(PrimitiveArrayView {
+                validity: None,
+                values: &[1, 2, 3, 4, 5, 6],
+            }),
+            FieldMeta {
+                name: String::from("foo"),
+                nullable: false,
+                metadata: Default::default(),
+            },
+        ),
+    ];
+
+    // first type has an unused element
+    let view = ArrayView::DenseUnion(DenseUnionArrayView {
+        types: &[0, 0, 1],
+        offsets: &[0, 2, 0],
+        fields: fields.clone(),
+    });
+    assert_error_contains(
+        &ArrayDeserializer::new(String::from("foo"), None, view),
+        "consecutive offsets",
+    );
+
+    // first type has an unused element
+    let view = ArrayView::DenseUnion(DenseUnionArrayView {
+        types: &[0, 0, 0],
+        offsets: &[0, 1, 4],
+        fields: fields.clone(),
+    });
+    assert_error_contains(
+        &ArrayDeserializer::new(String::from("foo"), None, view),
+        "consecutive offsets",
+    );
+}

serde_arrow/src/test/mod.rs

@@ -1,4 +1,5 @@
 mod api_chrono;
+mod deserialization;
 mod error_messages;
 mod jiff;
 mod schema_like;