Repository navigation
Add outer nullability to Union #8798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,23 +79,8 @@ 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<DType> { | ||
| 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()); | ||
| } | ||
| _ => {} | ||
| if let (DType::Union(lhs, lhs_null), DType::Union(rhs, rhs_null)) = (self, other) { | ||
| return (lhs == rhs).then(|| DType::Union(lhs.clone(), *lhs_null | *rhs_null)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these rules will go away. They need a replacement but essentially this has to be part of the higher level that integrates with vortex |
||
| } | ||
|
|
||
| let union_null = self.nullability() | other.nullability(); | ||
|
|
@@ -377,13 +362,15 @@ mod tests { | |
| vec![DType::Primitive(PType::I32, NonNullable)], | ||
| ) | ||
| .unwrap(), | ||
| NonNullable, | ||
| ); | ||
| let nullable = DType::Union( | ||
| UnionVariants::new( | ||
| ["value"].into(), | ||
| vec![DType::Primitive(PType::I32, Nullable)], | ||
| ) | ||
| .unwrap(), | ||
| NonNullable, | ||
| ); | ||
|
|
||
| assert_eq!( | ||
|
|
@@ -395,29 +382,22 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn least_supertype_null_requires_nullable_union() { | ||
| fn least_supertype_null_makes_union_outer_nullable() { | ||
| let nonnullable = DType::Union( | ||
| UnionVariants::new( | ||
| ["value"].into(), | ||
| vec![DType::Primitive(PType::I32, NonNullable)], | ||
| ) | ||
| .unwrap(), | ||
| NonNullable, | ||
| ); | ||
| let nullable = DType::Union( | ||
| UnionVariants::new( | ||
| ["value"].into(), | ||
| vec![DType::Primitive(PType::I32, Nullable)], | ||
| ) | ||
| .unwrap(), | ||
| ); | ||
| let nullable = nonnullable.as_nullable(); | ||
|
|
||
| assert!(DType::Null.least_supertype(&nonnullable).is_none()); | ||
| assert!(nonnullable.least_supertype(&DType::Null).is_none()); | ||
| assert_eq!( | ||
| DType::Null.least_supertype(&nullable), | ||
| DType::Null.least_supertype(&nonnullable), | ||
| Some(nullable.clone()) | ||
| ); | ||
| assert_eq!(nullable.least_supertype(&DType::Null), Some(nullable)); | ||
| assert_eq!(nonnullable.least_supertype(&DType::Null), Some(nullable)); | ||
| } | ||
|
|
||
| #[test] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,8 +64,8 @@ impl DType { | |
| | List(_, null) | ||
| | FixedSizeList(_, _, null) | ||
| | Struct(_, null) | ||
| | Union(_, null) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With unions now reporting only their outer nullability here, Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually wrong by our new definition of null unions, as we say outer null value for union is not the same as a union of nullable things |
||
| | Variant(null) => matches!(null, Nullability::Nullable), | ||
| Union(variants) => variants.derived_nullability().is_nullable(), | ||
| Extension(ext_dtype) => ext_dtype.storage_dtype().is_nullable(), | ||
| } | ||
| } | ||
|
|
@@ -82,8 +82,7 @@ impl DType { | |
|
|
||
| /// 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`]. | ||
| /// [`DType::Null`] has intrinsic nullability and is returned unchanged. | ||
| pub fn with_nullability(&self, nullability: Nullability) -> Self { | ||
| match self { | ||
| Null => Null, | ||
|
|
@@ -95,7 +94,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()), | ||
| Union(vs, _) => Union(vs.clone(), nullability), | ||
| Variant(_) => Variant(nullability), | ||
| Extension(ext) => Extension(ext.with_nullability(nullability)), | ||
| } | ||
|
|
@@ -127,7 +126,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() | ||
|
|
@@ -439,12 +438,20 @@ 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<UnionVariants> { | ||
| if let Union(uv) = self { Some(uv) } else { None } | ||
| if let Union(uv, _) = self { | ||
| Some(uv) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| /// Downcast a `DType` to an `ExtDType` | ||
|
|
@@ -498,7 +505,7 @@ impl Display for DType { | |
| .map(|(field_null, dt)| format!("{field_null}={dt}")) | ||
| .join(", "), | ||
| ), | ||
| Union(uv) => write!(f, "union({uv}){}", uv.derived_nullability()), | ||
| Union(uv, null) => write!(f, "union({uv}){null}"), | ||
| Variant(null) => write!(f, "variant{null}"), | ||
| Extension(ext) => write!(f, "{}", ext), | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so... do we do the fun thing where we try to construct a union more cleverly by looking at the types and type ids / field names?
I think there is a strong argument to do this, but also we can do this another time