diff --git a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs index 1d04a074d6f..6b2feb380a7 100644 --- a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs @@ -289,7 +289,7 @@ fn supports_uncompressed_size_in_bytes(dtype: &DType) -> bool { DType::Struct(fields, _) => fields .fields() .all(|field| supports_uncompressed_size_in_bytes(&field)), - DType::Union(variants, _) => variants + DType::Union(variants) => variants .variants() .all(|variant| supports_uncompressed_size_in_bytes(&variant)), DType::Variant(_) => false, diff --git a/vortex-array/src/dtype/coercion.rs b/vortex-array/src/dtype/coercion.rs index d57c0b9c1f4..3df356d93af 100644 --- a/vortex-array/src/dtype/coercion.rs +++ b/vortex-array/src/dtype/coercion.rs @@ -79,6 +79,25 @@ impl DType { /// The core primitive — what type can hold both `self` and `other`? /// Returns `None` if no common supertype exists. pub fn least_supertype(&self, other: &DType) -> Option { + match (self, other) { + (DType::Union(lhs), DType::Union(rhs)) => { + return (lhs == rhs).then(|| self.clone()); + } + (DType::Null, DType::Union(variants)) => { + return variants + .derived_nullability() + .is_nullable() + .then(|| other.clone()); + } + (DType::Union(variants), DType::Null) => { + return variants + .derived_nullability() + .is_nullable() + .then(|| self.clone()); + } + _ => {} + } + let union_null = self.nullability() | other.nullability(); if let ( @@ -308,6 +327,7 @@ mod tests { use crate::dtype::DType; use crate::dtype::PType; + use crate::dtype::UnionVariants; use crate::dtype::decimal::DecimalDType; use crate::dtype::nullability::Nullability::NonNullable; use crate::dtype::nullability::Nullability::Nullable; @@ -349,6 +369,57 @@ mod tests { ); } + #[test] + fn least_supertype_union_requires_identical_variants() { + let nonnullable = DType::Union( + UnionVariants::new( + ["value"].into(), + vec![DType::Primitive(PType::I32, NonNullable)], + ) + .unwrap(), + ); + let nullable = DType::Union( + UnionVariants::new( + ["value"].into(), + vec![DType::Primitive(PType::I32, Nullable)], + ) + .unwrap(), + ); + + assert_eq!( + nonnullable.least_supertype(&nonnullable), + Some(nonnullable.clone()) + ); + assert!(nonnullable.least_supertype(&nullable).is_none()); + assert!(nullable.least_supertype(&nonnullable).is_none()); + } + + #[test] + fn least_supertype_null_requires_nullable_union() { + let nonnullable = DType::Union( + UnionVariants::new( + ["value"].into(), + vec![DType::Primitive(PType::I32, NonNullable)], + ) + .unwrap(), + ); + let nullable = DType::Union( + UnionVariants::new( + ["value"].into(), + vec![DType::Primitive(PType::I32, Nullable)], + ) + .unwrap(), + ); + + assert!(DType::Null.least_supertype(&nonnullable).is_none()); + assert!(nonnullable.least_supertype(&DType::Null).is_none()); + assert_eq!( + DType::Null.least_supertype(&nullable), + Some(nullable.clone()) + ); + assert_eq!(nullable.least_supertype(&DType::Null), Some(nullable)); + } + #[test] fn least_supertype_unsigned_widening() { let u8_nn = DType::Primitive(PType::U8, NonNullable); diff --git a/vortex-array/src/dtype/dtype_impl.rs b/vortex-array/src/dtype/dtype_impl.rs index e13262fc168..5767a124667 100644 --- a/vortex-array/src/dtype/dtype_impl.rs +++ b/vortex-array/src/dtype/dtype_impl.rs @@ -64,8 +64,8 @@ impl DType { | List(_, null) | FixedSizeList(_, _, null) | Struct(_, null) - | Union(_, null) | Variant(null) => matches!(null, Nullability::Nullable), + Union(variants) => variants.derived_nullability().is_nullable(), Extension(ext_dtype) => ext_dtype.storage_dtype().is_nullable(), } } @@ -80,7 +80,10 @@ impl DType { self.with_nullability(Nullability::Nullable) } - /// Get a new DType with the given nullability (but otherwise the same as `self`) + /// Get a new DType with the given nullability (but otherwise the same as `self`). + /// + /// [`DType::Null`] and [`DType::Union`] have intrinsic nullability and are returned unchanged. + /// To change a union's nullability, construct different [`UnionVariants`]. pub fn with_nullability(&self, nullability: Nullability) -> Self { match self { Null => Null, @@ -92,7 +95,7 @@ impl DType { List(edt, _) => List(Arc::clone(edt), nullability), FixedSizeList(edt, size, _) => FixedSizeList(Arc::clone(edt), *size, nullability), Struct(sf, _) => Struct(sf.clone(), nullability), - Union(vs, _) => Union(vs.clone(), nullability), + Union(vs) => Union(vs.clone()), Variant(_) => Variant(nullability), Extension(ext) => Extension(ext.with_nullability(nullability)), } @@ -124,7 +127,7 @@ impl DType { .zip_eq(rhs_dtype.fields()) .all(|(l, r)| l.eq_ignore_nullability(&r))) } - (Union(lhs, _), Union(rhs, _)) => { + (Union(lhs), Union(rhs)) => { // Equal `names` implies equal length by FieldNames equality. lhs.names() == rhs.names() && lhs.type_ids() == rhs.type_ids() @@ -436,20 +439,12 @@ impl DType { /// Get the [`UnionVariants`] if `self` is a [`DType::Union`], otherwise `None`. pub fn as_union_variants_opt(&self) -> Option<&UnionVariants> { - if let Union(uv, _) = self { - Some(uv) - } else { - None - } + if let Union(uv) = self { Some(uv) } else { None } } /// Owned version of [Self::as_union_variants_opt]. pub fn into_union_variants_opt(self) -> Option { - if let Union(uv, _) = self { - Some(uv) - } else { - None - } + if let Union(uv) = self { Some(uv) } else { None } } /// Downcast a `DType` to an `ExtDType` @@ -503,7 +498,7 @@ impl Display for DType { .map(|(field_null, dt)| format!("{field_null}={dt}")) .join(", "), ), - Union(uv, null) => write!(f, "union({uv}){null}"), + Union(uv) => write!(f, "union({uv}){}", uv.derived_nullability()), Variant(null) => write!(f, "variant{null}"), Extension(ext) => write!(f, "{}", ext), } diff --git a/vortex-array/src/dtype/mod.rs b/vortex-array/src/dtype/mod.rs index 403531779aa..6a166a868ff 100644 --- a/vortex-array/src/dtype/mod.rs +++ b/vortex-array/src/dtype/mod.rs @@ -114,8 +114,13 @@ pub enum DType { /// A `Union` is composed of one or more **variants**, each with a name and a `DType`. A per-row /// `i8` tag selects which variant is "live" at that row. /// + /// Unlike other nested types, a union has no independent outer nullability. Its nullability is + /// derived at runtime from its variants: a union is nullable when any variant can contain null. + /// A concrete union array has no parent validity bitmap; a row's validity is the validity of + /// its selected child. + /// /// See [`UnionVariants`] for the type-tag conventions and accessors. - Union(UnionVariants, Nullability), + Union(UnionVariants), /// Dynamically typed values stored as Vortex scalars. /// @@ -148,7 +153,7 @@ impl PartialEq for DType { // StructFields handles its own Arc::ptr_eq in its PartialEq impl. (Self::Struct(a, na), Self::Struct(b, nb)) => na == nb && a == b, // UnionVariants handles its own Arc::ptr_eq in its PartialEq impl. - (Self::Union(a, na), Self::Union(b, nb)) => na == nb && a == b, + (Self::Union(a), Self::Union(b)) => a == b, (Self::Variant(a), Self::Variant(b)) => a == b, (Self::Extension(a), Self::Extension(b)) => a == b, // Every variant is listed in the first position so that adding a new diff --git a/vortex-array/src/dtype/serde/flatbuffers.rs b/vortex-array/src/dtype/serde/flatbuffers.rs index 803f92b6795..eef15d59c57 100644 --- a/vortex-array/src/dtype/serde/flatbuffers.rs +++ b/vortex-array/src/dtype/serde/flatbuffers.rs @@ -11,7 +11,6 @@ use itertools::Itertools; use vortex_error::VortexError; use vortex_error::VortexResult; use vortex_error::vortex_bail; -use vortex_error::vortex_ensure; use vortex_error::vortex_err; use vortex_flatbuffers::FlatBuffer; use vortex_flatbuffers::FlatBufferRoot; @@ -22,7 +21,6 @@ use vortex_session::VortexSession; use crate::dtype::DType; use crate::dtype::DecimalDType; use crate::dtype::FieldDType; -use crate::dtype::Nullability; use crate::dtype::PType; use crate::dtype::StructFields; use crate::dtype::UnionVariants; @@ -236,15 +234,7 @@ impl TryFrom for DType { .ok_or_else(|| vortex_err!("failed to parse union from flatbuffer"))?; let variants = UnionVariants::from_fb(fb_union, vfdt.buffer().clone(), vfdt.session.clone())?; - - let nullability: Nullability = fb_union.nullable().into(); - vortex_ensure!( - variants.nullability_constraints_satisfied(nullability), - "Union nullability constraint not satisfied: nullability={:?}", - nullability - ); - - Ok(Self::Union(variants, nullability)) + Ok(Self::Union(variants)) } fb::Type::Variant => { let fb_variant = fb @@ -390,7 +380,7 @@ impl WriteFlatBuffer for DType { ) .as_union_value() } - Self::Union(uv, n) => { + Self::Union(uv) => { let names = uv .names() .iter() @@ -412,7 +402,6 @@ impl WriteFlatBuffer for DType { names, dtypes, type_ids, - nullable: (*n).into(), }, ) .as_union_value() @@ -583,7 +572,6 @@ mod test { ], ) .unwrap(), - Nullability::NonNullable, ) } @@ -593,14 +581,13 @@ mod test { } #[test] - fn test_union_round_trip_flatbuffer_with_nullability() { + fn test_union_round_trip_flatbuffer_with_nullable_variant() { let dtype = DType::Union( UnionVariants::new( ["null_variant", "str"].into(), vec![DType::Null, DType::Utf8(Nullability::NonNullable)], ) .unwrap(), - Nullability::Nullable, ); roundtrip_dtype(dtype); } @@ -618,7 +605,6 @@ mod test { vec![0, 5, 7], ) .unwrap(), - Nullability::NonNullable, ); let bytes = dtype.write_flatbuffer_bytes().unwrap(); @@ -631,7 +617,7 @@ mod test { let deserialized = DType::try_from(view).unwrap(); assert_eq!(dtype, deserialized); - let DType::Union(uv, _) = &deserialized else { + let DType::Union(uv) = &deserialized else { panic!("Expected Union"); }; assert_eq!(uv.type_ids(), &[0, 5, 7]); @@ -654,7 +640,6 @@ mod test { vec![DType::Utf8(Nullability::NonNullable), struct_with_union], ) .unwrap(), - Nullability::NonNullable, ); roundtrip_dtype(outer_union); @@ -710,7 +695,6 @@ mod test { names: Some(names), dtypes: Some(dtypes), type_ids: Some(type_ids), - nullable: false, }, ); diff --git a/vortex-array/src/dtype/serde/mod.rs b/vortex-array/src/dtype/serde/mod.rs index d0f43f0c4ac..17e07c244cb 100644 --- a/vortex-array/src/dtype/serde/mod.rs +++ b/vortex-array/src/dtype/serde/mod.rs @@ -24,6 +24,7 @@ mod test { use crate::dtype::DType; use crate::dtype::Nullability; use crate::dtype::PType; + use crate::dtype::UnionVariants; use crate::dtype::serde::DTypeSerde; use crate::dtype::test::SESSION; @@ -181,4 +182,24 @@ mod test { .unwrap(); assert_eq!(DType::Variant(Nullability::Nullable), deserialized); } + + #[test] + fn test_serde_union_dtype_json_roundtrip() { + let dtype = DType::Union( + UnionVariants::new(["value"].into(), vec![DType::Utf8(Nullability::Nullable)]).unwrap(), + ); + + let json = serde_json::to_string(&dtype).unwrap(); + assert_eq!( + json, + r#"{"Union":{"names":["value"],"dtypes":[{"Utf8":true}],"type_ids":[0]}}"# + ); + let mut deserializer = serde_json::Deserializer::from_str(&json); + let deserialized: DType = DTypeSerde::::new(&SESSION) + .deserialize(&mut deserializer) + .unwrap(); + + assert_eq!(deserialized, dtype); + assert_eq!(deserialized.nullability(), Nullability::Nullable); + } } diff --git a/vortex-array/src/dtype/serde/proto.rs b/vortex-array/src/dtype/serde/proto.rs index ba33259a3a5..e3dfb90557a 100644 --- a/vortex-array/src/dtype/serde/proto.rs +++ b/vortex-array/src/dtype/serde/proto.rs @@ -5,13 +5,11 @@ use std::sync::Arc; use vortex_error::VortexError; use vortex_error::VortexResult; -use vortex_error::vortex_ensure; use vortex_error::vortex_err; use vortex_session::VortexSession; use crate::dtype::DType; use crate::dtype::DecimalDType; -use crate::dtype::Nullability; use crate::dtype::PType; use crate::dtype::StructFields; use crate::dtype::UnionVariants; @@ -106,15 +104,7 @@ impl DType { }) .collect::>>()?; let variants = UnionVariants::try_new(names, dtypes, type_ids)?; - - let nullability: Nullability = u.nullable.into(); - vortex_ensure!( - variants.nullability_constraints_satisfied(nullability), - "Union nullability constraint not satisfied: nullability={:?}", - nullability - ); - - Ok(Self::Union(variants, nullability)) + Ok(Self::Union(variants)) } DtypeType::Variant(v) => Ok(Self::Variant(v.nullable.into())), DtypeType::Extension(e) => { @@ -183,14 +173,13 @@ impl TryFrom<&DType> for pb::DType { .collect::>>()?, nullable: (*null).into(), }), - DType::Union(uv, null) => DtypeType::Union(pb::Union { + DType::Union(uv) => DtypeType::Union(pb::Union { names: uv.names().iter().map(|n| n.as_ref().to_string()).collect(), dtypes: uv .variants() .map(|d| Self::try_from(&d)) .collect::>>()?, type_ids: uv.type_ids().iter().map(|t| *t as i32).collect(), - nullable: (*null).into(), }), DType::Variant(null) => DtypeType::Variant(pb::Variant { nullable: (*null).into(), @@ -427,23 +416,22 @@ mod tests { ], ) .unwrap(); - let dtype = DType::Union(variants, Nullability::NonNullable); + let dtype = DType::Union(variants); let converted = round_trip_dtype(&dtype); assert_eq!(dtype, converted); } #[test] - fn test_union_round_trip_proto_with_nullability() { + fn test_union_round_trip_proto_with_nullable_variant() { let variants = UnionVariants::new( ["null_variant", "str"].into(), vec![DType::Null, DType::Utf8(Nullability::NonNullable)], ) .unwrap(); - assert!(variants.nullability_constraints_satisfied(Nullability::Nullable)); - assert!(!variants.nullability_constraints_satisfied(Nullability::NonNullable)); + assert_eq!(variants.derived_nullability(), Nullability::Nullable); - let dtype = DType::Union(variants, Nullability::Nullable); + let dtype = DType::Union(variants); let converted = round_trip_dtype(&dtype); assert_eq!(dtype, converted); } @@ -461,43 +449,17 @@ mod tests { ) .unwrap(); - let dtype = DType::Union(variants, Nullability::NonNullable); + let dtype = DType::Union(variants); let converted = round_trip_dtype(&dtype); assert_eq!(dtype, converted); - let DType::Union(uv, _) = &converted else { + let DType::Union(uv) = &converted else { panic!("Expected Union"); }; assert_eq!(uv.type_ids(), &[0, 5, 7]); assert!(!uv.is_consecutive()); } - #[test] - fn test_union_proto_rejects_violating_nullability_constraint() { - // Nullable Union with no nullable or Null children must be rejected. - let proto = pb::DType { - dtype_type: Some(DtypeType::Union(pb::Union { - names: vec!["a".to_string(), "b".to_string()], - dtypes: vec![ - pb::DType::try_from(&DType::Primitive(PType::I32, Nullability::NonNullable)) - .unwrap(), - pb::DType::try_from(&DType::Utf8(Nullability::NonNullable)).unwrap(), - ], - type_ids: vec![0, 1], - nullable: true, - })), - }; - - let result = DType::from_proto(&proto, &SESSION); - assert!(result.is_err()); - assert!( - result - .unwrap_err() - .to_string() - .contains("Union nullability constraint not satisfied") - ); - } - #[test] fn test_union_proto_rejects_out_of_range_type_id() { let proto = pb::DType { @@ -509,7 +471,6 @@ mod tests { ], // 200 does not fit in i8. type_ids: vec![200], - nullable: false, })), }; @@ -534,7 +495,6 @@ mod tests { ], ) .unwrap(), - Nullability::NonNullable, ); let struct_with_union = DType::Struct( @@ -551,7 +511,6 @@ mod tests { vec![DType::Utf8(Nullability::NonNullable), struct_with_union], ) .unwrap(), - Nullability::NonNullable, ); let converted = round_trip_dtype(&outer_union); diff --git a/vortex-array/src/dtype/serde/serde.rs b/vortex-array/src/dtype/serde/serde.rs index 9d127047b40..1d386a34cbf 100644 --- a/vortex-array/src/dtype/serde/serde.rs +++ b/vortex-array/src/dtype/serde/serde.rs @@ -116,12 +116,7 @@ impl Serialize for DType { state.serialize_field(n)?; state.end() } - DType::Union(uv, n) => { - let mut state = serializer.serialize_tuple_variant("DType", 11, "Union", 2)?; - state.serialize_field(&uv)?; - state.serialize_field(n)?; - state.end() - } + DType::Union(uv) => serializer.serialize_newtype_variant("DType", 11, "Union", uv), DType::Variant(n) => serializer.serialize_newtype_variant("DType", 10, "Variant", n), DType::Extension(ext) => { serializer.serialize_newtype_variant("DType", 9, "Extension", ext) @@ -237,9 +232,9 @@ impl<'de> DeserializeSeed<'de> for DTypeSerde<'_, DType> { "Struct" => access.newtype_variant_seed(StructFieldsSeed { session: self.session, }), - "Union" => access.newtype_variant_seed(UnionVariantsSeed { - session: self.session, - }), + "Union" => access + .newtype_variant_seed(DTypeSerde::::new(self.session)) + .map(DType::Union), "Variant" => { let n = access.newtype_variant()?; Ok(DType::Variant(n)) @@ -410,57 +405,6 @@ impl<'de> DeserializeSeed<'de> for StructFieldsSeed<'_> { } } -struct UnionVariantsSeed<'a> { - session: &'a VortexSession, -} - -impl<'de> DeserializeSeed<'de> for UnionVariantsSeed<'_> { - type Value = DType; - - fn deserialize(self, deserializer: D) -> Result - where - D: Deserializer<'de>, - { - struct UnionVisitor<'a> { - session: &'a VortexSession, - } - - impl<'de> Visitor<'de> for UnionVisitor<'_> { - type Value = DType; - - fn expecting(&self, f: &mut Formatter) -> fmt::Result { - f.write_str("Union tuple (variants, nullability)") - } - - fn visit_seq(self, mut seq: A) -> Result - where - A: SeqAccess<'de>, - { - let variants = seq - .next_element_seed(DTypeSerde::::new(self.session))? - .ok_or_else(|| de::Error::invalid_length(0, &self))?; - let nullability: Nullability = seq - .next_element()? - .ok_or_else(|| de::Error::invalid_length(1, &self))?; - if !variants.nullability_constraints_satisfied(nullability) { - return Err(de::Error::custom(format!( - "Union nullability constraint not satisfied: nullability={:?}", - nullability - ))); - } - Ok(DType::Union(variants, nullability)) - } - } - - deserializer.deserialize_tuple( - 2, - UnionVisitor { - session: self.session, - }, - ) - } -} - impl<'de> DeserializeSeed<'de> for DTypeSerde<'_, UnionVariants> { type Value = UnionVariants; diff --git a/vortex-array/src/dtype/union.rs b/vortex-array/src/dtype/union.rs index cb443e87d36..c2097d3994c 100644 --- a/vortex-array/src/dtype/union.rs +++ b/vortex-array/src/dtype/union.rs @@ -339,26 +339,12 @@ impl UnionVariants { self.0.type_ids[index] } - /// Checks whether this set of variants satisfies the constraint imposed by a union-level - /// `Nullability`: + /// Returns the runtime-derived nullability of the union. /// - /// - `Nullable`: at least one variant is `DType::Null` or has nullable nullability. - /// - `NonNullable`: no `DType::Null` variants and no nullable variants. - /// - /// The check materializes each [`FieldDType`] (so it may be expensive if some are still - /// flatbuffer-backed views). - /// - /// It is intentionally not part of `UnionVariants` construction since `Nullability` lives on - /// the `DType::Union(_, Nullability)` enum variant, not on `UnionVariants` itself. - pub fn nullability_constraints_satisfied(&self, union_nullability: Nullability) -> bool { - let has_null_or_nullable = self - .variants() - .any(|dt| matches!(dt, DType::Null) || dt.is_nullable()); - - match union_nullability { - Nullability::Nullable => has_null_or_nullable, - Nullability::NonNullable => !has_null_or_nullable, - } + /// This is not a zero-cost accessor: it scans every variant and materializes each + /// [`FieldDType`], which may be expensive when variants are still flatbuffer-backed views. + pub fn derived_nullability(&self) -> Nullability { + self.variants().any(|dtype| dtype.is_nullable()).into() } } @@ -480,7 +466,6 @@ mod tests { DType::Primitive(PType::I32, Nullability::NonNullable), ], Nullability::Nullable, - true, )] #[case::nullable_with_nullable_child( vec![ @@ -488,57 +473,57 @@ mod tests { DType::Utf8(Nullability::Nullable), ], Nullability::Nullable, - true, )] - #[case::nullable_with_no_null_or_nullable( + #[case::nonnullable( vec![ DType::Primitive(PType::I32, Nullability::NonNullable), DType::Utf8(Nullability::NonNullable), ], - Nullability::Nullable, - false, - )] - #[case::nonnullable_with_null_child( - vec![ - DType::Null, - DType::Primitive(PType::I32, Nullability::NonNullable), - ], - Nullability::NonNullable, - false, - )] - #[case::nonnullable_with_nullable_child( - vec![ - DType::Primitive(PType::I32, Nullability::NonNullable), - DType::Utf8(Nullability::Nullable), - ], Nullability::NonNullable, - false, )] - #[case::nonnullable_clean( - vec![ - DType::Primitive(PType::I32, Nullability::NonNullable), - DType::Utf8(Nullability::NonNullable), - ], - Nullability::NonNullable, - true, - )] - fn test_nullability_constraints( - #[case] dtypes: Vec, - #[case] nullability: Nullability, - #[case] expected: bool, - ) { + #[case::empty(vec![], Nullability::NonNullable)] + fn test_derived_nullability(#[case] dtypes: Vec, #[case] expected: Nullability) { let names: Vec<&str> = (0..dtypes.len()).map(|i| ["a", "b", "c", "d"][i]).collect(); let variants = UnionVariants::new(names.as_slice().into(), dtypes).unwrap(); - assert_eq!( - variants.nullability_constraints_satisfied(nullability), - expected + assert_eq!(variants.derived_nullability(), expected); + } + + #[test] + fn test_nested_union_nullability() { + let inner = DType::Union( + UnionVariants::new(["value"].into(), vec![DType::Utf8(Nullability::Nullable)]).unwrap(), + ); + let outer = DType::Union( + UnionVariants::new( + ["nested", "number"].into(), + vec![ + inner, + DType::Primitive(PType::I32, Nullability::NonNullable), + ], + ) + .unwrap(), + ); + + assert_eq!(outer.nullability(), Nullability::Nullable); + } + + #[test] + fn test_with_nullability_does_not_change_union_variants() { + let nonnullable = DType::Union(i32_variants()); + assert_eq!(nonnullable.as_nullable(), nonnullable); + assert_eq!(nonnullable.nullability(), Nullability::NonNullable); + + let nullable = DType::Union( + UnionVariants::new(["value"].into(), vec![DType::Utf8(Nullability::Nullable)]).unwrap(), ); + assert_eq!(nullable.as_nonnullable(), nullable); + assert_eq!(nullable.nullability(), Nullability::Nullable); } #[test] fn test_display() { let variants = i32_variants(); - let dtype = DType::Union(variants, Nullability::NonNullable); + let dtype = DType::Union(variants); assert_eq!(dtype.to_string(), "union(int=i32, str=utf8)"); let nullable = DType::Union( @@ -550,7 +535,6 @@ mod tests { ], ) .unwrap(), - Nullability::Nullable, ); assert_eq!(nullable.to_string(), "union(int=i32, maybe_str=utf8?)?"); } @@ -567,7 +551,7 @@ mod tests { vec![0, 5, 7], ) .unwrap(); - let dtype = DType::Union(variants, Nullability::NonNullable); + let dtype = DType::Union(variants); assert_eq!(dtype.to_string(), "union(a@0=i32, b@5=utf8, c@7=bool)"); } diff --git a/vortex-datafusion/src/convert/scalars.rs b/vortex-datafusion/src/convert/scalars.rs index 1e11dd6eede..e26aa005adc 100644 --- a/vortex-datafusion/src/convert/scalars.rs +++ b/vortex-datafusion/src/convert/scalars.rs @@ -832,8 +832,7 @@ mod tests { ["a"].into(), vec![DType::Primitive(PType::I32, Nullability::Nullable)], ) - .unwrap(), - Nullability::Nullable + .unwrap() )))] fn unsupported_vortex_scalars_return_errors(#[case] scalar: Scalar) { let err = scalar.try_to_df().unwrap_err(); diff --git a/vortex-flatbuffers/flatbuffers/vortex-dtype/dtype.fbs b/vortex-flatbuffers/flatbuffers/vortex-dtype/dtype.fbs index fe737eafe0f..add1ae63e2e 100644 --- a/vortex-flatbuffers/flatbuffers/vortex-dtype/dtype.fbs +++ b/vortex-flatbuffers/flatbuffers/vortex-dtype/dtype.fbs @@ -67,13 +67,10 @@ table Variant { nullable: bool; } -// Explicit ids keep `nullable` at id 0, the id it had before the variant metadata fields were -// added, so schemas written in between remain readable. table Union { - names: [string] (id: 1); - dtypes: [DType] (id: 2); - type_ids: [byte] (id: 3); // length must equal dtypes.len() - nullable: bool (id: 0); + names: [string]; + dtypes: [DType]; + type_ids: [byte]; // length must equal dtypes.len() } union Type { diff --git a/vortex-flatbuffers/src/generated/dtype.rs b/vortex-flatbuffers/src/generated/dtype.rs index 8b26f68dc34..953e826da82 100644 --- a/vortex-flatbuffers/src/generated/dtype.rs +++ b/vortex-flatbuffers/src/generated/dtype.rs @@ -1477,10 +1477,9 @@ impl<'a> ::flatbuffers::Follow<'a> for Union<'a> { } impl<'a> Union<'a> { - pub const VT_NULLABLE: ::flatbuffers::VOffsetT = 4; - pub const VT_NAMES: ::flatbuffers::VOffsetT = 6; - pub const VT_DTYPES: ::flatbuffers::VOffsetT = 8; - pub const VT_TYPE_IDS: ::flatbuffers::VOffsetT = 10; + pub const VT_NAMES: ::flatbuffers::VOffsetT = 4; + pub const VT_DTYPES: ::flatbuffers::VOffsetT = 6; + pub const VT_TYPE_IDS: ::flatbuffers::VOffsetT = 8; #[inline] pub unsafe fn init_from_table(table: ::flatbuffers::Table<'a>) -> Self { @@ -1495,18 +1494,10 @@ impl<'a> Union<'a> { if let Some(x) = args.type_ids { builder.add_type_ids(x); } if let Some(x) = args.dtypes { builder.add_dtypes(x); } if let Some(x) = args.names { builder.add_names(x); } - builder.add_nullable(args.nullable); builder.finish() } - #[inline] - pub fn nullable(&self) -> bool { - // Safety: - // Created from valid Table for this object - // which contains a valid value in this slot - unsafe { self._tab.get::(Union::VT_NULLABLE, Some(false)).unwrap()} - } #[inline] pub fn names(&self) -> Option<::flatbuffers::Vector<'a, ::flatbuffers::ForwardsUOffset<&'a str>>> { // Safety: @@ -1536,7 +1527,6 @@ impl ::flatbuffers::Verifiable for Union<'_> { v: &mut ::flatbuffers::Verifier, pos: usize ) -> Result<(), ::flatbuffers::InvalidFlatbuffer> { v.visit_table(pos)? - .visit_field::("nullable", Self::VT_NULLABLE, false)? .visit_field::<::flatbuffers::ForwardsUOffset<::flatbuffers::Vector<'_, ::flatbuffers::ForwardsUOffset<&'_ str>>>>("names", Self::VT_NAMES, false)? .visit_field::<::flatbuffers::ForwardsUOffset<::flatbuffers::Vector<'_, ::flatbuffers::ForwardsUOffset>>>("dtypes", Self::VT_DTYPES, false)? .visit_field::<::flatbuffers::ForwardsUOffset<::flatbuffers::Vector<'_, i8>>>("type_ids", Self::VT_TYPE_IDS, false)? @@ -1545,7 +1535,6 @@ impl ::flatbuffers::Verifiable for Union<'_> { } } pub struct UnionArgs<'a> { - pub nullable: bool, pub names: Option<::flatbuffers::WIPOffset<::flatbuffers::Vector<'a, ::flatbuffers::ForwardsUOffset<&'a str>>>>, pub dtypes: Option<::flatbuffers::WIPOffset<::flatbuffers::Vector<'a, ::flatbuffers::ForwardsUOffset>>>>, pub type_ids: Option<::flatbuffers::WIPOffset<::flatbuffers::Vector<'a, i8>>>, @@ -1554,7 +1543,6 @@ impl<'a> Default for UnionArgs<'a> { #[inline] fn default() -> Self { UnionArgs { - nullable: false, names: None, dtypes: None, type_ids: None, @@ -1567,10 +1555,6 @@ pub struct UnionBuilder<'a: 'b, 'b, A: ::flatbuffers::Allocator + 'a> { start_: ::flatbuffers::WIPOffset<::flatbuffers::TableUnfinishedWIPOffset>, } impl<'a: 'b, 'b, A: ::flatbuffers::Allocator + 'a> UnionBuilder<'a, 'b, A> { - #[inline] - pub fn add_nullable(&mut self, nullable: bool) { - self.fbb_.push_slot::(Union::VT_NULLABLE, nullable, false); - } #[inline] pub fn add_names(&mut self, names: ::flatbuffers::WIPOffset<::flatbuffers::Vector<'b , ::flatbuffers::ForwardsUOffset<&'b str>>>) { self.fbb_.push_slot_always::<::flatbuffers::WIPOffset<_>>(Union::VT_NAMES, names); @@ -1601,7 +1585,6 @@ impl<'a: 'b, 'b, A: ::flatbuffers::Allocator + 'a> UnionBuilder<'a, 'b, A> { impl ::core::fmt::Debug for Union<'_> { fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { let mut ds = f.debug_struct("Union"); - ds.field("nullable", &self.nullable()); ds.field("names", &self.names()); ds.field("dtypes", &self.dtypes()); ds.field("type_ids", &self.type_ids()); diff --git a/vortex-flatbuffers/src/generated/message.rs b/vortex-flatbuffers/src/generated/message.rs index 9cc48fe6be6..b3e4be76ef7 100644 --- a/vortex-flatbuffers/src/generated/message.rs +++ b/vortex-flatbuffers/src/generated/message.rs @@ -2,8 +2,8 @@ // @generated extern crate alloc; -use crate::dtype::*; use crate::array::*; +use crate::dtype::*; #[deprecated(since = "2.0.0", note = "Use associated constants instead. This will no longer be generated in 2021.")] pub const ENUM_MIN_MESSAGE_VERSION: u8 = 0; diff --git a/vortex-proto/proto/dtype.proto b/vortex-proto/proto/dtype.proto index 4a221cef043..decc2179080 100644 --- a/vortex-proto/proto/dtype.proto +++ b/vortex-proto/proto/dtype.proto @@ -78,7 +78,6 @@ message Union { repeated string names = 1; repeated DType dtypes = 2; repeated int32 type_ids = 3; // length must equal dtypes.len(); each value must fit in int8 - bool nullable = 4; } message DType { diff --git a/vortex-proto/src/generated/vortex.dtype.rs b/vortex-proto/src/generated/vortex.dtype.rs index 27e44089253..4bffca278e2 100644 --- a/vortex-proto/src/generated/vortex.dtype.rs +++ b/vortex-proto/src/generated/vortex.dtype.rs @@ -80,8 +80,6 @@ pub struct Union { /// length must equal dtypes.len(); each value must fit in int8 #[prost(int32, repeated, tag = "3")] pub type_ids: ::prost::alloc::vec::Vec, - #[prost(bool, tag = "4")] - pub nullable: bool, } #[derive(Clone, PartialEq, ::prost::Message)] pub struct DType {