From 75d61fda786e237188d7e487c8c668bf89474e26 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 21 Jul 2026 12:07:50 -0400 Subject: [PATCH] prevent non empty unions and clarify null docs Signed-off-by: Connor Tsui --- vortex-array/src/dtype/dtype_impl.rs | 7 +- vortex-array/src/dtype/union.rs | 60 +++++++-------- vortex-array/src/scalar/constructor.rs | 4 +- vortex-array/src/scalar/downcast.rs | 8 +- vortex-array/src/scalar/proto.rs | 2 +- vortex-array/src/scalar/scalar_impl.rs | 43 ++++++++--- vortex-array/src/scalar/scalar_value.rs | 26 ++++++- vortex-array/src/scalar/tests/casting.rs | 10 +-- vortex-array/src/scalar/tests/primitives.rs | 84 +++++++++++++++++---- vortex-array/src/scalar/typed_view/union.rs | 57 +++++++++----- vortex-array/src/scalar/validate.rs | 2 +- 11 files changed, 215 insertions(+), 88 deletions(-) diff --git a/vortex-array/src/dtype/dtype_impl.rs b/vortex-array/src/dtype/dtype_impl.rs index 52408db66b7..7de16913c6b 100644 --- a/vortex-array/src/dtype/dtype_impl.rs +++ b/vortex-array/src/dtype/dtype_impl.rs @@ -445,7 +445,10 @@ impl DType { } } - /// Get the [`UnionVariants`] if `self` is a [`DType::Union`], otherwise `None`. + /// Get the [`UnionVariants`] if `self` is a [`DType::Union`], otherwise [`None`]. + /// + /// This [`Option`] only represents whether the dtype is a union; Union nullability does not + /// affect the result. pub fn as_union_variants_opt(&self) -> Option<&UnionVariants> { if let Union(uv, _) = self { Some(uv) @@ -454,7 +457,7 @@ impl DType { } } - /// Owned version of [Self::as_union_variants_opt]. + /// Owned version of [`Self::as_union_variants_opt`]. pub fn into_union_variants_opt(self) -> Option { if let Union(uv, _) = self { Some(uv) diff --git a/vortex-array/src/dtype/union.rs b/vortex-array/src/dtype/union.rs index 6074e32e87a..30df46e2d80 100644 --- a/vortex-array/src/dtype/union.rs +++ b/vortex-array/src/dtype/union.rs @@ -19,8 +19,9 @@ use crate::dtype::FieldNames; /// Type information for a union array. /// /// A `UnionVariants` describes the possible alternative types for each row of a union, along with a -/// per-variant `u8` type tag. We use the term **variants** (rather than "fields") because a union -/// is a sum type: each row chooses exactly one alternative. +/// per-variant `u8` type tag. A union must contain at least one variant. We use the term +/// **variants** (rather than "fields") because a union is a sum type: each row chooses exactly one +/// alternative. /// /// By default, tag `i` selects the child at offset `i` (`type_ids = [0, 1, ..., N-1]`). Vortex uses /// unsigned tags and supports up to 256 variants. @@ -164,22 +165,7 @@ impl UnionVariants { } } -impl Default for UnionVariants { - fn default() -> Self { - Self::empty() - } -} - impl UnionVariants { - /// The variants of the empty union. - pub fn empty() -> Self { - Self(Arc::new(UnionVariantsInner::from_fields( - FieldNames::default(), - Arc::from([]), - Arc::from([]), - ))) - } - /// Validate that `names`, `dtypes`, and `type_ids` are mutually consistent. fn validate_shape(names: &FieldNames, n_dtypes: usize, type_ids: &[u8]) -> VortexResult<()> { vortex_ensure_eq!( @@ -196,6 +182,10 @@ impl UnionVariants { names.len(), type_ids.len() ); + vortex_ensure!( + !names.is_empty(), + "union must have at least one variant (for now)" + ); vortex_ensure!( type_ids.iter().all_unique(), @@ -221,8 +211,9 @@ impl UnionVariants { /// /// # Errors /// - /// Returns an error if names, dtypes, or type IDs do not all have the same length, or if there - /// are any duplicate names or type IDs, or if there are more than 256 variants. + /// Returns an error if the union has no variants, if names, dtypes, or type IDs do not all have + /// the same length, if there are any duplicate names or type IDs, or if there are more than 256 + /// variants. pub fn try_new(names: FieldNames, dtypes: Vec, type_ids: Vec) -> VortexResult { Self::validate_shape(&names, dtypes.len(), &type_ids)?; @@ -238,8 +229,8 @@ impl UnionVariants { /// /// # Errors /// - /// `names` and `dtypes` must have the same length, and `names.len()` cannot be more than - /// `u8::MAX as usize + 1` (256). + /// The union must have at least one variant, `names` and `dtypes` must have the same length, and + /// `names.len()` cannot be more than `u8::MAX as usize + 1` (256). pub fn new(names: FieldNames, dtypes: Vec) -> VortexResult { const MAX_VARIANTS: usize = u8::MAX as usize + 1; vortex_ensure!( @@ -265,8 +256,9 @@ impl UnionVariants { /// /// # Errors /// - /// Returns an error if names, dtypes, or type IDs do not all have the same length, or if there - /// are any duplicate names or type IDs, or if there are more than 256 variants. + /// Returns an error if the union has no variants, if names, dtypes, or type IDs do not all have + /// the same length, if there are any duplicate names or type IDs, or if there are more than 256 + /// variants. pub(crate) fn try_from_fields( names: FieldNames, dtypes: Vec, @@ -330,7 +322,7 @@ impl UnionVariants { ) } - /// Get the [`DType`] of a variant by offset. + /// Get the [`DType`] of a variant by offset, or [`None`] if `index` is out of bounds. pub fn variant_by_index(&self, index: usize) -> Option { Some( self.0 @@ -600,11 +592,19 @@ mod tests { } #[test] - fn test_empty() { - let v = UnionVariants::empty(); - assert!(v.is_empty()); - assert_eq!(v.len(), 0); - assert_eq!(v.type_ids(), &[] as &[u8]); - assert!(v.is_consecutive()); + fn test_empty_rejected() { + let error = UnionVariants::new(FieldNames::default(), vec![]).unwrap_err(); + assert!( + error + .to_string() + .contains("union must have at least one variant") + ); + + let error = UnionVariants::try_new(FieldNames::default(), vec![], vec![]).unwrap_err(); + assert!( + error + .to_string() + .contains("union must have at least one variant") + ); } } diff --git a/vortex-array/src/scalar/constructor.rs b/vortex-array/src/scalar/constructor.rs index 76cbd8f36d7..234e95441a7 100644 --- a/vortex-array/src/scalar/constructor.rs +++ b/vortex-array/src/scalar/constructor.rs @@ -198,7 +198,9 @@ impl Scalar { /// Creates a union scalar from a type ID and child scalar. /// /// The selected child's dtype is verified and then discarded. Its raw value is stored so an - /// inner null child remains distinct from a null at the outer union level. + /// inner null child remains distinct from a null at the outer union level. Passing a null child + /// creates a non-null union with a selected type ID; use [`Scalar::null`] with a nullable + /// [`DType::Union`] to create an outer null union with no selected type ID. /// /// # Errors /// diff --git a/vortex-array/src/scalar/downcast.rs b/vortex-array/src/scalar/downcast.rs index 2d36ca3e0f7..2cdb67fade2 100644 --- a/vortex-array/src/scalar/downcast.rs +++ b/vortex-array/src/scalar/downcast.rs @@ -168,7 +168,13 @@ impl Scalar { .vortex_expect("Failed to convert scalar to union") } - /// Returns a view of the scalar as a union scalar if it has a union type. + /// Returns a view of the scalar as a union scalar if it has a union dtype. + /// + /// [`None`] means the scalar's dtype is not [`DType::Union`](crate::dtype::DType); it does not + /// describe the scalar's nullness. + /// + /// An outer null union still returns `Some(UnionScalar)` and can be inspected with + /// [`UnionScalar::is_null`](crate::scalar::UnionScalar::is_null). pub fn as_union_opt(&self) -> Option> { if !self.dtype().is_union() { return None; diff --git a/vortex-array/src/scalar/proto.rs b/vortex-array/src/scalar/proto.rs index 62d032c1676..d3f7894f86e 100644 --- a/vortex-array/src/scalar/proto.rs +++ b/vortex-array/src/scalar/proto.rs @@ -116,7 +116,7 @@ impl From<&ScalarValue> for pb::ScalarValue { ScalarValue::Union(v) => pb::ScalarValue { kind: Some(Kind::UnionValue(Box::new(PbUnionValue { type_id: u32::from(v.type_id()), - value: Some(Box::new(ScalarValue::to_proto(v.value()))), + value: Some(Box::new(ScalarValue::to_proto(v.child_value()))), }))), }, ScalarValue::Variant(v) => pb::ScalarValue { diff --git a/vortex-array/src/scalar/scalar_impl.rs b/vortex-array/src/scalar/scalar_impl.rs index cb85b51b114..a4351932152 100644 --- a/vortex-array/src/scalar/scalar_impl.rs +++ b/vortex-array/src/scalar/scalar_impl.rs @@ -101,10 +101,16 @@ impl Scalar { /// See [`Scalar::zero_value`] for more details about "zero" values. /// /// For non-nullable nested types, this function recursively creates valid default children. + /// For a union specifically: + /// + /// - A nullable union defaults to an **outer null**. It has no selected variant or type ID. + /// - A non-nullable union selects its first variant. That selected child may itself default to + /// an **inner null** when its dtype is nullable; the enclosing union remains non-null and + /// retains the first variant's type ID. /// /// # Panics /// - /// Panics if `dtype` has no default value, such as a non-nullable union. + /// Panics if `dtype` has no default value. pub fn default_value(dtype: &DType) -> Self { Self::try_default_value(dtype) .unwrap_or_else(|| vortex_panic!("{dtype} has no default value")) @@ -133,16 +139,19 @@ impl Scalar { /// element [`DType`] /// - `Struct`: A struct where each field has a zero value, which is determined by the field /// [`DType`] - /// - `Union`: Does not have a "zero" value + /// - `Union`: A non-null union selecting the first variant with a non-null child zero value /// - `Extension`: The zero value of the storage [`DType`] - /// /// # Panics /// - /// Panics if the dtype has no non-null zero value, such as `Null` or a union. + /// Panics if the dtype has no non-null zero value, such as `Null`, or if a nested dtype needed + /// to construct the zero value has no non-null zero value. + /// + /// Unlike [`Scalar::default_value`], this never uses either an outer or inner null for a union: + /// even a nullable union's zero value is non-null and contains a non-null selected child. pub fn zero_value(dtype: &DType) -> Self { let value = ScalarValue::zero_value(dtype); - // SAFETY: We assume that `zero_value` creates a valid `ScalarValue` for the `DType`. + // SAFETY: `zero_value` creates a valid `ScalarValue` for the `DType`. unsafe { Self::new_unchecked(dtype.clone(), Some(value)) } } @@ -186,8 +195,12 @@ impl Scalar { /// Returns `true` if the [`Scalar`] has a non-null zero value. /// - /// Returns `None` if the scalar is null, otherwise returns `Some(true)` if the value is zero - /// and `Some(false)` otherwise. + /// Returns `None` if the scalar is null or its zero-ness is undefined. Otherwise, returns + /// `Some(true)` if the value is zero and `Some(false)` if it is not. + /// + /// A union that selects a variant other than the first returns `Some(false)`. A union selecting + /// its first variant delegates to that child's [`Scalar::is_zero`], so an inner null child + /// returns `None` and a non-null zero child returns `Some(true)`. pub fn is_zero(&self) -> Option { let value = self.value()?; @@ -212,8 +225,16 @@ impl Scalar { .as_struct() .fields_iter() .is_some_and(|mut fields| fields.all(|f| f.is_zero() == Some(true))), - // We do not define union to have any valid "zero" value. - DType::Union(..) => false, + // Only the first variant of unions can be zero. Its child determines whether the + // zero-ness is true, false, or undefined. + DType::Union(..) => { + let union = self.as_union(); + if union.child_index() != Some(0) { + false + } else { + union.child().and_then(|child| child.is_zero())? + } + } DType::Variant(_) => self.as_variant().is_zero()?, DType::Extension(_) => self.as_extension().to_storage_scalar().is_zero()?, }; @@ -284,7 +305,7 @@ impl Scalar { .unwrap_or_default(), DType::Union(..) => self .as_union() - .value() + .child() .map_or(0, |value| 1 + value.approx_nbytes()), DType::Variant(_) => self.as_variant().value().map_or(0, Scalar::approx_nbytes), DType::Extension(_) => self.as_extension().to_storage_scalar().approx_nbytes(), @@ -403,7 +424,7 @@ fn partial_cmp_non_null_scalar_values( let child_index = variants.tag_to_child_index(lhs.type_id())?; let child_dtype = variants.variant_by_index(child_index)?; - partial_cmp_scalar_values(&child_dtype, lhs.value(), rhs.value()) + partial_cmp_scalar_values(&child_dtype, lhs.child_value(), rhs.child_value()) } // Variant values can have a different dtype in each row, so it doesn't make sense to // compare them. diff --git a/vortex-array/src/scalar/scalar_value.rs b/vortex-array/src/scalar/scalar_value.rs index f042573f088..c898832d96c 100644 --- a/vortex-array/src/scalar/scalar_value.rs +++ b/vortex-array/src/scalar/scalar_value.rs @@ -9,6 +9,7 @@ use std::fmt::Formatter; use itertools::Itertools; use vortex_buffer::BufferString; use vortex_buffer::ByteBuffer; +use vortex_error::VortexExpect; use vortex_error::vortex_panic; use crate::dtype::DType; @@ -51,7 +52,7 @@ impl ScalarValue { } /// Returns the non-null zero value for `dtype`, or [`None`] if no such value exists. - fn try_zero_value(dtype: &DType) -> Option { + pub(super) fn try_zero_value(dtype: &DType) -> Option { Some(match dtype { DType::Null => return None, DType::Bool(_) => Self::Bool(false), @@ -73,7 +74,17 @@ impl ScalarValue { .collect::>>()?; Self::Tuple(field_values) } - DType::Union(..) => return None, + DType::Union(variants, _) => { + let child_dtype = variants + .variant_by_index(0) + .vortex_expect("union must have at least one variant"); + let child_value = Self::try_zero_value(&child_dtype)?; + + Self::Union(UnionValue::new( + variants.child_index_to_tag(0), + Some(child_value), + )) + } DType::Variant(_) => Self::Variant(Box::new(Scalar::null(DType::Null))), DType::Extension(ext_dtype) => { // Since we have no way to define a "zero" extension value (since we have no idea @@ -114,7 +125,14 @@ impl ScalarValue { .collect::>>()?; Self::Tuple(field_values) } - DType::Union(..) => return None, + DType::Union(variants, _) => { + let child_dtype = variants + .variant_by_index(0) + .vortex_expect("union must have at least one variant"); + let child_value = Self::try_default_value(&child_dtype)?; + + Self::Union(UnionValue::new(variants.child_index_to_tag(0), child_value)) + } DType::Variant(_) => Self::Variant(Box::new(Scalar::null(DType::Null))), DType::Extension(ext_dtype) => { // Since we have no way to define a "default" extension value (since we have no idea @@ -174,7 +192,7 @@ impl Display for ScalarValue { } ScalarValue::Union(value) => { write!(f, "union@{}(", value.type_id())?; - match value.value() { + match value.child_value() { Some(value) => write!(f, "{value}"), None => write!(f, "null"), }?; diff --git a/vortex-array/src/scalar/tests/casting.rs b/vortex-array/src/scalar/tests/casting.rs index a21e7193497..ce8f0fa217d 100644 --- a/vortex-array/src/scalar/tests/casting.rs +++ b/vortex-array/src/scalar/tests/casting.rs @@ -420,7 +420,7 @@ mod tests { assert_eq!( unchanged_child .as_union() - .value() + .child() .map(|scalar| scalar.dtype().clone()), Some(DType::Primitive(PType::I32, Nullability::NonNullable)) ); @@ -428,7 +428,7 @@ mod tests { assert_eq!( changed_child .as_union() - .value() + .child() .map(|scalar| scalar.dtype().clone()), Some(DType::Utf8(Nullability::Nullable)) ); @@ -482,7 +482,7 @@ mod tests { let cast = scalar.cast(&target_dtype)?; let selected_child = cast .as_union() - .value() + .child() .vortex_expect("present union must have a selected child"); assert_eq!(cast.dtype(), &target_dtype); @@ -531,7 +531,7 @@ mod tests { assert_eq!(cast.dtype(), &target_dtype); assert_eq!(union.dtype(), &target_union_dtype); assert_eq!( - union.as_union().value(), + union.as_union().child(), Some(Scalar::primitive(42_i32, Nullability::Nullable)) ); @@ -605,7 +605,7 @@ mod tests { assert_eq!( nullable .as_union() - .value() + .child() .map(|scalar| scalar.dtype().clone()), Some(DType::Primitive(PType::I32, Nullability::NonNullable)) ); diff --git a/vortex-array/src/scalar/tests/primitives.rs b/vortex-array/src/scalar/tests/primitives.rs index f1742af4475..028dd99876d 100644 --- a/vortex-array/src/scalar/tests/primitives.rs +++ b/vortex-array/src/scalar/tests/primitives.rs @@ -11,7 +11,6 @@ mod tests { use std::sync::Arc; use vortex_buffer::ByteBuffer; - use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_utils::aliases::hash_set::HashSet; @@ -95,41 +94,96 @@ mod tests { } #[test] - #[should_panic(expected = "has no default value")] - fn default_value_for_non_nullable_union_panics() { + fn default_value_for_non_nullable_union_selects_first_variant() -> VortexResult<()> { let non_nullable = DType::Union( - union_variants(Nullability::NonNullable, Nullability::NonNullable) - .vortex_expect("union variants must be valid"), + union_variants(Nullability::Nullable, Nullability::NonNullable)?, Nullability::NonNullable, ); - Scalar::default_value(&non_nullable); + let scalar = Scalar::default_value(&non_nullable); + let union = scalar.as_union(); + + assert!(!scalar.is_null()); + assert!(!union.is_null()); + assert_eq!(union.type_id(), Some(5)); + assert!(union.child().is_some_and(|child| child.is_null())); + assert_eq!(scalar.is_zero(), None); + + Ok(()) } #[test] - #[should_panic(expected = "has no non-null zero value")] - fn union_has_no_zero_value() { + fn union_zero_value_selects_first_variant() -> VortexResult<()> { let dtype = DType::Union( - union_variants(Nullability::NonNullable, Nullability::NonNullable) - .vortex_expect("union variants must be valid"), - Nullability::NonNullable, + union_variants(Nullability::Nullable, Nullability::NonNullable)?, + Nullability::Nullable, ); - Scalar::zero_value(&dtype); + let scalar = Scalar::zero_value(&dtype); + let union = scalar.as_union(); + + assert!(!scalar.is_null()); + assert!(!union.is_null()); + assert_eq!(union.type_id(), Some(5)); + assert_eq!( + union.child(), + Some(Scalar::primitive(0_i32, Nullability::Nullable)) + ); + assert_eq!(scalar.is_zero(), Some(true)); + + Ok(()) } #[test] - fn union_is_zero_does_not_inspect_selected_child() -> VortexResult<()> { - let variants = union_variants(Nullability::NonNullable, Nullability::NonNullable)?; + fn union_has_no_zero_value_when_first_variant_is_null() -> VortexResult<()> { + let variants = UnionVariants::try_new( + ["null", "int"].into(), + vec![ + DType::Null, + DType::Primitive(PType::I32, Nullability::NonNullable), + ], + vec![5, 9], + )?; + let dtype = DType::Union(variants.clone(), Nullability::NonNullable); let scalar = Scalar::union( + variants, + 5, + Scalar::null(DType::Null), + Nullability::NonNullable, + )?; + + assert_eq!(ScalarValue::try_zero_value(&dtype), None); + assert_eq!(scalar.is_zero(), None); + + Ok(()) + } + + #[test] + fn union_is_zero_requires_first_variant_and_zero_child() -> VortexResult<()> { + let variants = union_variants(Nullability::NonNullable, Nullability::NonNullable)?; + let first_zero = Scalar::union( variants.clone(), 5, Scalar::from(0_i32), Nullability::NonNullable, )?; + let first_nonzero = Scalar::union( + variants.clone(), + 5, + Scalar::from(1_i32), + Nullability::NonNullable, + )?; + let second_zero = Scalar::union( + variants.clone(), + 9, + Scalar::from(""), + Nullability::NonNullable, + )?; let null = Scalar::null(DType::Union(variants, Nullability::Nullable)); - assert_eq!(scalar.is_zero(), Some(false)); + assert_eq!(first_zero.is_zero(), Some(true)); + assert_eq!(first_nonzero.is_zero(), Some(false)); + assert_eq!(second_zero.is_zero(), Some(false)); assert_eq!(null.is_zero(), None); Ok(()) diff --git a/vortex-array/src/scalar/typed_view/union.rs b/vortex-array/src/scalar/typed_view/union.rs index aa570563b26..60ff96dbb78 100644 --- a/vortex-array/src/scalar/typed_view/union.rs +++ b/vortex-array/src/scalar/typed_view/union.rs @@ -49,9 +49,13 @@ impl UnionValue { self.type_id } - /// Returns the selected variant's raw value, or [`None`] if the selected child is null. + /// Returns the selected variant's raw value. + /// + /// [`None`] represents an inner null: the enclosing union is present and still has this + /// [`UnionValue`]'s type ID, but its selected child is null. Outer null unions have no + /// [`UnionValue`] and therefore cannot be observed through this method. #[inline] - pub fn value(&self) -> Option<&ScalarValue> { + pub fn child_value(&self) -> Option<&ScalarValue> { self.value.as_deref() } } @@ -60,17 +64,19 @@ impl UnionValue { /// /// A present union scalar carries a type ID and the raw value of the selected variant. An outer /// null union scalar has neither because outer nullness is represented by the enclosing [`Scalar`]. +/// A present union may select a nullable child whose value is an inner null; in that case the union +/// still has a type ID and [`Self::is_null`] returns `false`. #[derive(Debug, Clone, Copy)] pub struct UnionScalar<'a> { /// The data type of this scalar. dtype: &'a DType, - /// The selected union value, or [`None`] if the union scalar is null. + /// The selected union value, or [`None`] if the union scalar is outer null. value: Option<&'a UnionValue>, } impl Display for UnionScalar<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let Some(value) = self.value() else { + let Some(value) = self.child() else { return write!(f, "null"); }; let name = self @@ -92,6 +98,9 @@ impl Eq for UnionScalar<'_> {} impl<'a> UnionScalar<'a> { /// Attempts to create a union scalar view from a [`DType`] and optional [`ScalarValue`]. /// + /// A `None` value represents an outer null union. An inner null is represented by a present + /// [`ScalarValue::Union`] whose selected child value is `None`. + /// /// # Errors /// /// Returns an error if `dtype` and `value` do not form a valid union scalar. This includes a @@ -132,6 +141,8 @@ impl<'a> UnionScalar<'a> { } /// Returns the variants of this union scalar. + /// + /// The variants are part of the dtype and are available even when the union is outer null. #[inline] pub fn variants(&self) -> &'a UnionVariants { self.dtype @@ -145,37 +156,49 @@ impl<'a> UnionScalar<'a> { self.dtype.nullability() } - /// Returns true if this union scalar is null. + /// Returns true if this union scalar is outer null. + /// + /// This does not inspect the selected child. A non-null union containing an inner null child + /// returns `false`. #[inline] pub fn is_null(&self) -> bool { self.value.is_none() } - /// Returns the selected type ID, or `None` if this scalar is null. + /// Returns the selected type ID, or `None` if this scalar is outer null. + /// + /// A non-null union containing an inner null child still returns its selected type ID. #[inline] pub fn type_id(&self) -> Option { Some(self.value?.type_id()) } - /// Returns the selected variant's child index, or `None` if this scalar is null. + /// Returns the selected variant's child index, or [`None`] if this union is outer null. + /// + /// A union containing an inner null child is still present and returns `Some(index)`. pub fn child_index(&self) -> Option { self.variants().tag_to_child_index(self.type_id()?) } - /// Returns the selected variant's name, or `None` if this scalar is null. + /// Returns the selected variant's name, or [`None`] if this union is outer null. + /// + /// A union containing an inner null child is still present and returns `Some(name)`. pub fn variant_name(&self) -> Option<&'a FieldName> { self.variants().names().get(self.child_index()?) } - /// Returns the selected variant's dtype, or `None` if this scalar is null. + /// Returns the selected variant's dtype, or [`None`] if this union is outer null. + /// + /// A union containing an inner null child is still present and returns `Some(dtype)`. pub fn variant_dtype(&self) -> Option { self.variants().variant_by_index(self.child_index()?) } /// Returns the selected child scalar, or [`None`] if the outer union scalar is null. /// - /// A selected null child is returned as a present, null [`Scalar`]. - pub fn value(&self) -> Option { + /// The outer [`Option`] describes the union's outer nullness. A selected inner null child is + /// returned as `Some(child)`, where `child.is_null()` is `true`. + pub fn child(&self) -> Option { let union_value = self.value?; let child_dtype = self .variant_dtype() @@ -183,7 +206,7 @@ impl<'a> UnionScalar<'a> { // SAFETY: Construction of this `UnionScalar` guarantees that its type ID resolves to this // child dtype and that the raw child value recursively validates against it. - Some(unsafe { Scalar::new_unchecked(child_dtype, union_value.value().cloned()) }) + Some(unsafe { Scalar::new_unchecked(child_dtype, union_value.child_value().cloned()) }) } } @@ -223,13 +246,13 @@ mod tests { assert_eq!(union.child_index(), Some(0)); assert_eq!(union.variant_name().map(AsRef::as_ref), Some("int")); assert_eq!(union.nullability(), Nullability::Nullable); - assert_eq!(union.value(), Some(child)); + assert_eq!(union.child(), Some(child)); assert_eq!( scalar .value() .vortex_expect("union must have an outer value") .as_union() - .value(), + .child_value(), Some(&ScalarValue::Primitive(42_i32.into())) ); @@ -248,13 +271,13 @@ mod tests { let union = scalar.as_union(); assert!(!union.is_null()); assert_eq!(union.type_id(), Some(5)); - assert!(union.value().is_some_and(|scalar| scalar.is_null())); + assert!(union.child().is_some_and(|scalar| scalar.is_null())); assert!( scalar .value() .vortex_expect("union must have an outer value") .as_union() - .value() + .child_value() .is_none() ); @@ -262,7 +285,7 @@ mod tests { let outer_union = outer_null.as_union(); assert!(outer_union.is_null()); assert_eq!(outer_union.type_id(), None); - assert_eq!(outer_union.value(), None); + assert_eq!(outer_union.child(), None); Ok(()) } diff --git a/vortex-array/src/scalar/validate.rs b/vortex-array/src/scalar/validate.rs index 45d9677f361..f81147376b8 100644 --- a/vortex-array/src/scalar/validate.rs +++ b/vortex-array/src/scalar/validate.rs @@ -136,7 +136,7 @@ impl Scalar { .variant_by_index(child_index) .vortex_expect("resolved union child index must be valid"); - Self::validate(&child_dtype, union_value.value()).map_err(|error| { + Self::validate(&child_dtype, union_value.child_value()).map_err(|error| { vortex_error::vortex_err!( "union value for type ID {type_id} is invalid for dtype {child_dtype}: \ {error}"