diff --git a/vortex-array/src/dtype/serde/flatbuffers.rs b/vortex-array/src/dtype/serde/flatbuffers.rs index 8a1e7e6b611..efda432d7ab 100644 --- a/vortex-array/src/dtype/serde/flatbuffers.rs +++ b/vortex-array/src/dtype/serde/flatbuffers.rs @@ -89,15 +89,7 @@ impl StructFields { }) .collect::>(); - if names.len() != dtypes.len() { - vortex_bail!( - "length mismatch between struct names ({}) and dtypes ({})", - names.len(), - dtypes.len() - ); - } - - Ok(StructFields::from_fields(names, dtypes)) + StructFields::try_from_fields(names, dtypes) } } @@ -754,6 +746,111 @@ mod test { assert_eq!(viewed, eager); } + /// A struct field whose own dtype cannot be decoded must fail here, not later in + /// `StructFields::fields()` (#8848). + #[test] + fn test_struct_field_with_undecodable_dtype_errors() { + let mut fbb = FlatBufferBuilder::new(); + let name = fbb.create_string("bad"); + let names = fbb.create_vector(&[name]); + // `names` is optional in the schema, so the verifier accepts this struct but + // decoding it fails. + let inner_struct = fb::Struct_::create( + &mut fbb, + &fb::Struct_Args { + names: None, + dtypes: None, + nullable: false, + }, + ); + let bad_field = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Struct_, + type_: Some(inner_struct.as_union_value()), + }, + ); + let dtypes = fbb.create_vector(&[bad_field]); + let struct_table = fb::Struct_::create( + &mut fbb, + &fb::Struct_Args { + names: Some(names), + dtypes: Some(dtypes), + nullable: false, + }, + ); + let dtype = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Struct_, + type_: Some(struct_table.as_union_value()), + }, + ); + fbb.finish_minimal(dtype); + let (vec, start) = fbb.collapse(); + let end = vec.len(); + let buffer = FlatBuffer::align_from(ByteBuffer::from(vec).slice(start..end)); + + let root_fb = root::(&buffer).unwrap(); + let view = ViewedDType::from_fb_loc(root_fb._tab.loc(), buffer, SESSION.clone()); + + let err = DType::try_from(view).expect_err("undecodable field dtype must not parse"); + assert!( + err.to_string().contains("bad"), + "error should name the field: {err}" + ); + } + + /// The same on the union side. + #[test] + fn test_union_variant_with_undecodable_dtype_errors() { + let mut fbb = FlatBufferBuilder::new(); + let name = fbb.create_string("bad"); + let names = fbb.create_vector(&[name]); + let inner_struct = fb::Struct_::create( + &mut fbb, + &fb::Struct_Args { + names: None, + dtypes: None, + nullable: false, + }, + ); + let bad_variant = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Struct_, + type_: Some(inner_struct.as_union_value()), + }, + ); + let dtypes = fbb.create_vector(&[bad_variant]); + let type_ids = fbb.create_vector(&[0i8]); + let union_table = fb::Union::create( + &mut fbb, + &fb::UnionArgs { + names: Some(names), + dtypes: Some(dtypes), + type_ids: Some(type_ids), + nullable: false, + }, + ); + let dtype = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Union, + type_: Some(union_table.as_union_value()), + }, + ); + fbb.finish_minimal(dtype); + let (vec, start) = fbb.collapse(); + let end = vec.len(); + let buffer = FlatBuffer::align_from(ByteBuffer::from(vec).slice(start..end)); + + let root_fb = root::(&buffer).unwrap(); + let view = ViewedDType::from_fb_loc(root_fb._tab.loc(), buffer, SESSION.clone()); + + DType::try_from(view).expect_err("undecodable variant dtype must not parse"); + } + #[test] fn test_struct_malformed_flatbuffer() { let mut fbb = FlatBufferBuilder::new(); diff --git a/vortex-array/src/dtype/struct_.rs b/vortex-array/src/dtype/struct_.rs index 95f73295d52..5cb5b0271ca 100644 --- a/vortex-array/src/dtype/struct_.rs +++ b/vortex-array/src/dtype/struct_.rs @@ -319,6 +319,38 @@ impl StructFields { Self::from_fields(names, dtypes) } + /// Create a new [`StructFields`] from names and [`FieldDType`]s, decoding every field + /// dtype up front. + /// + /// Deserialization must use this rather than [`Self::from_fields`]: a [`FieldDType`] + /// backed by a view decodes lazily, and the accessors that decode it return a plain + /// [`DType`], so a field dtype that fails to decode has nowhere to surface but a panic. + /// + /// # Errors + /// + /// Returns an error if `names` and `dtypes` differ in length, or if any field dtype + /// cannot be decoded. + pub fn try_from_fields(names: FieldNames, dtypes: Vec) -> VortexResult { + if names.len() != dtypes.len() { + vortex_bail!( + "length mismatch between names ({}) and dtypes ({})", + names.len(), + dtypes.len() + ); + } + + for (name, dtype) in names.iter().zip_eq(dtypes.iter()) { + dtype + .value() + .map_err(|e| e.with_context(format!("invalid dtype for struct field {name}")))?; + } + + Ok(Self(Arc::new(StructFieldsInner::from_fields( + names, + dtypes.into(), + )))) + } + /// Create a new [`StructFields`] from a list of names and [`FieldDType`] which can be either lazily or eagerly serialized. pub fn from_fields(names: FieldNames, dtypes: Vec) -> Self { if names.len() != dtypes.len() { diff --git a/vortex-array/src/dtype/union.rs b/vortex-array/src/dtype/union.rs index 30df46e2d80..1b94e4905e7 100644 --- a/vortex-array/src/dtype/union.rs +++ b/vortex-array/src/dtype/union.rs @@ -266,6 +266,13 @@ impl UnionVariants { ) -> VortexResult { Self::validate_shape(&names, dtypes.len(), &type_ids)?; + // Decode up front for the same reason as `StructFields::try_from_fields`. + for (name, dtype) in names.iter().zip_eq(dtypes.iter()) { + dtype + .value() + .map_err(|e| e.with_context(format!("invalid dtype for union variant {name}")))?; + } + Ok(Self(Arc::new(UnionVariantsInner::from_fields( names, dtypes.into(),