Skip to content

Commit

Permalink
Merge pull request #217 from chmp/feature/37-improve-error-messages
Browse files Browse the repository at this point in the history 
Feature/37 improve error messages for serialization
  • Loading branch information
@chmp
chmp authored Sep 1, 2024
2 parents 4d7c4a8 + d93a05d commit 16866ac
Show file tree
Hide file tree
Showing 53 changed files with 1,504 additions and 953 deletions.
12 changes: 12 additions & 0 deletions Development.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,15 @@ modules can can be run without installing further packages.

1. `python x.py add-arrow-version {VERSION}`
2. `python x.py precommit`

## Error format

Style:

- Use uppercase letters to start the error message
- Do not include trailing punctuation (e.g., "Not supported", not "Not supported.")

Common annotations:

- `field`: the path of the field affected by the error
- `data_type`: the Arrow data type of the field affected by the error
10 changes: 5 additions & 5 deletions serde_arrow/src/arrow2_impl/array.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ impl<'a> TryFrom<&'a dyn A2Array> for ArrayView<'a> {
);
};
Ok(V::List(ListArrayView {
meta: meta_from_field(field.as_ref().try_into()?)?,
meta: meta_from_field(field.as_ref().try_into()?),
validity: bits_with_offset_from_bitmap(array.validity()),
offsets: array.offsets().as_slice(),
element: Box::new(array.values().as_ref().try_into()?),
Expand All @@ -311,7 +311,7 @@ impl<'a> TryFrom<&'a dyn A2Array> for ArrayView<'a> {
);
};
Ok(V::LargeList(ListArrayView {
meta: meta_from_field(field.as_ref().try_into()?)?,
meta: meta_from_field(field.as_ref().try_into()?),
validity: bits_with_offset_from_bitmap(array.validity()),
offsets: array.offsets().as_slice(),
element: Box::new(array.values().as_ref().try_into()?),
Expand All @@ -327,7 +327,7 @@ impl<'a> TryFrom<&'a dyn A2Array> for ArrayView<'a> {
for (child_field, child) in child_fields.iter().zip(array.values()) {
fields.push((
child.as_ref().try_into()?,
meta_from_field(child_field.try_into()?)?,
meta_from_field(child_field.try_into()?),
));
}
Ok(V::Struct(StructArrayView {
Expand All @@ -342,7 +342,7 @@ impl<'a> TryFrom<&'a dyn A2Array> for ArrayView<'a> {
array.data_type(),
);
};
let meta = meta_from_field(field.as_ref().try_into()?)?;
let meta = meta_from_field(field.as_ref().try_into()?);
let element: ArrayView<'_> = array.field().as_ref().try_into()?;

Ok(V::Map(ListArrayView {
Expand Down Expand Up @@ -378,7 +378,7 @@ impl<'a> TryFrom<&'a dyn A2Array> for ArrayView<'a> {
fields.push((
(*type_id).try_into()?,
child.as_ref().try_into()?,
meta_from_field(child_field.try_into()?)?,
meta_from_field(child_field.try_into()?),
));
}

Expand Down
12 changes: 6 additions & 6 deletions serde_arrow/src/arrow_impl/array.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ impl<'a> TryFrom<&'a dyn Array> for ArrayView<'a> {
Ok(ArrayView::List(ListArrayView {
validity: get_bits_with_offset(array),
offsets: array.value_offsets(),
meta: meta_from_field(field.as_ref().try_into()?)?,
meta: meta_from_field(field.as_ref().try_into()?),
element: Box::new(array.values().as_ref().try_into()?),
}))
} else if let Some(array) = any.downcast_ref::<GenericListArray<i64>>() {
Expand All @@ -443,7 +443,7 @@ impl<'a> TryFrom<&'a dyn Array> for ArrayView<'a> {
Ok(ArrayView::LargeList(ListArrayView {
validity: get_bits_with_offset(array),
offsets: array.value_offsets(),
meta: meta_from_field(field.as_ref().try_into()?)?,
meta: meta_from_field(field.as_ref().try_into()?),
element: Box::new(array.values().as_ref().try_into()?),
}))
} else if let Some(array) = any.downcast_ref::<FixedSizeListArray>() {
Expand All @@ -454,7 +454,7 @@ impl<'a> TryFrom<&'a dyn Array> for ArrayView<'a> {
len: array.len(),
n: *n,
validity: get_bits_with_offset(array),
meta: meta_from_field(field.as_ref().try_into()?)?,
meta: meta_from_field(field.as_ref().try_into()?),
element: Box::new(array.values().as_ref().try_into()?),
}))
} else if let Some(array) = any.downcast_ref::<StructArray>() {
Expand All @@ -465,7 +465,7 @@ impl<'a> TryFrom<&'a dyn Array> for ArrayView<'a> {
let mut fields = Vec::new();
for (field, array) in std::iter::zip(column_fields, array.columns()) {
let view = ArrayView::try_from(array.as_ref())?;
let meta = meta_from_field(Field::try_from(field.as_ref())?)?;
let meta = meta_from_field(Field::try_from(field.as_ref())?);
fields.push((view, meta));
}

Expand All @@ -483,7 +483,7 @@ impl<'a> TryFrom<&'a dyn Array> for ArrayView<'a> {
Ok(ArrayView::Map(ListArrayView {
validity: get_bits_with_offset(array),
offsets: array.value_offsets(),
meta: meta_from_field(Field::try_from(entries_field.as_ref())?)?,
meta: meta_from_field(Field::try_from(entries_field.as_ref())?),
element: Box::new(entries_array.try_into()?),
}))
} else if let Some(array) = any.downcast_ref::<DictionaryArray<UInt8Type>>() {
Expand All @@ -509,7 +509,7 @@ impl<'a> TryFrom<&'a dyn Array> for ArrayView<'a> {

let mut fields = Vec::new();
for (type_id, field) in union_fields.iter() {
let meta = meta_from_field(Field::try_from(field.as_ref())?)?;
let meta = meta_from_field(Field::try_from(field.as_ref())?);
let view: ArrayView = array.child(type_id).as_ref().try_into()?;
fields.push((type_id, view, meta));
}
Expand Down
2 changes: 1 addition & 1 deletion serde_arrow/src/arrow_impl/type_support.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::internal::{

impl From<ArrowError> for Error {
fn from(err: ArrowError) -> Self {
Self::custom(err.to_string())
Self::custom_from(err.to_string(), err)
}
}

Expand Down
9 changes: 5 additions & 4 deletions serde_arrow/src/internal/deserialization/string_deserializer.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::internal::{
arrow::BytesArrayView,
error::{error, fail, Result},
error::{fail, Result},
utils::{Mut, Offset},
};

Expand Down Expand Up @@ -39,9 +39,10 @@ impl<'a, O: Offset> StringDeserializer<'a, O> {
}

pub fn next_required(&mut self) -> Result<&'a str> {
self.next()?.ok_or_else(|| {
error!("Tried to deserialize a value from StringDeserializer, but value is missing")
})
let Some(next) = self.next()? else {
fail!("Tried to deserialize a value from StringDeserializer, but value is missing")
};
Ok(next)
}

pub fn peek_next(&self) -> Result<bool> {
Expand Down
7 changes: 5 additions & 2 deletions serde_arrow/src/internal/deserialization/utils.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::internal::{
arrow::BitsWithOffset,
error::{error, fail, Result},
error::{fail, Result},
utils::Offset,
};

Expand Down Expand Up @@ -44,7 +44,10 @@ impl<'a, T: Copy> ArrayBufferIterator<'a, T> {
}

pub fn next_required(&mut self) -> Result<T> {
self.next()?.ok_or_else(|| error!("missing value"))
let Some(next) = self.next()? else {
fail!("missing value");
};
Ok(next)
}

pub fn peek_next(&self) -> Result<bool> {
Expand Down
50 changes: 25 additions & 25 deletions serde_arrow/src/internal/deserializer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,19 +81,19 @@ impl<'de> serde::de::Deserializer<'de> for Deserializer<'de> {
}

fn deserialize_bool<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single bools")
fail!("Cannot deserialize single bools")
}

fn deserialize_byte_buf<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize byte buffers")
fail!("Cannot deserialize byte buffers")
}

fn deserialize_bytes<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize byte arrays")
fail!("Cannot deserialize byte arrays")
}

fn deserialize_char<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single chars")
fail!("Cannot deserialize single chars")
}

fn deserialize_enum<V: Visitor<'de>>(
Expand All @@ -102,55 +102,55 @@ impl<'de> serde::de::Deserializer<'de> for Deserializer<'de> {
_: &'static [&'static str],
_: V,
) -> Result<V::Value> {
fail!("cannot deserialize single enums")
fail!("Cannot deserialize single enums")
}

fn deserialize_f32<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single floats")
fail!("Cannot deserialize single floats")
}

fn deserialize_f64<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single floats")
fail!("Cannot deserialize single floats")
}

fn deserialize_i128<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_i16<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_i32<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_i64<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_i8<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_identifier<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single identifiers")
fail!("Cannot deserialize single identifiers")
}

fn deserialize_map<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single maps")
fail!("Cannot deserialize single maps")
}

fn deserialize_option<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single options")
fail!("Cannot deserialize single options")
}

fn deserialize_str<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single strings")
fail!("Cannot deserialize single strings")
}

fn deserialize_string<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single strings")
fail!("Cannot deserialize single strings")
}

fn deserialize_struct<V: Visitor<'de>>(
Expand All @@ -159,35 +159,35 @@ impl<'de> serde::de::Deserializer<'de> for Deserializer<'de> {
_: &'static [&'static str],
_: V,
) -> Result<V::Value> {
fail!("cannot deserialize single structs")
fail!("Cannot deserialize single structs")
}

fn deserialize_u128<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_u16<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_u32<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_u64<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_u8<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single integers")
fail!("Cannot deserialize single integers")
}

fn deserialize_unit<V: Visitor<'de>>(self, _: V) -> Result<V::Value> {
fail!("cannot deserialize single units")
fail!("Cannot deserialize single units")
}

fn deserialize_unit_struct<V: Visitor<'de>>(self, _: &'static str, _: V) -> Result<V::Value> {
fail!("cannot deserialize single units")
fail!("Cannot deserialize single units")
}

fn is_human_readable(&self) -> bool {
Expand Down
Loading

0 comments on commit 16866ac

Please sign in to comment.