Add UnionScalar#8739
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
7e619d3 to
243969b
Compare
| // Union nullability is part of its variant dtypes, so the generic nullability-only cast | ||
| // below is not valid for unions. Keep all non-identity union casts unsupported until their | ||
| // semantics are defined. | ||
| if matches!(self.dtype(), DType::Union(_)) || matches!(target_dtype, DType::Union(_)) { | ||
| vortex_bail!( | ||
| "non-identity union scalar cast from {} to {target_dtype} is not supported", | ||
| self.dtype() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Interested in what people think about how we should deal with this
8bb8db6 to
7925944
Compare
| /// We implement `Hash` manually so top-level dtype nullability does not affect the hash. | ||
| impl Hash for Scalar { | ||
| fn hash<H: Hasher>(&self, state: &mut H) { | ||
| // TODO(connor): Recursively ignore dtype nullability to match Scalar equality (#8744). |
43d2fc9 to
9de7663
Compare
9de7663 to
4893177
Compare
| // Union nullability is part of its variant dtypes, so the generic nullability-only cast | ||
| // below is not valid for unions. Keep all non-identity union casts unsupported until their | ||
| // semantics are defined. | ||
| if self.dtype().is_union() || target_dtype.is_union() { |
There was a problem hiding this comment.
I think you have to gate the other branch and make sure this falls back to union scalar
There was a problem hiding this comment.
what is the other branch?
There was a problem hiding this comment.
the one the comment refers to
There was a problem hiding this comment.
I don't think I understand, are you saying that it can somehow get past this gate?
There was a problem hiding this comment.
Basically @connortsui20 we should plumb this method fully such that we delegate to the match and fail there
| DType::List(..) | DType::FixedSizeList(..) => self.as_list().cast(target_dtype), | ||
| DType::Struct(..) => self.as_struct().cast(target_dtype), | ||
| DType::Union(..) => todo!("TODO(connor)[Union]: unimplemented"), | ||
| DType::Union(..) => unreachable!("union casts are handled before scalar dispatch"), |
There was a problem hiding this comment.
@connortsui20 I am saying you should fallback here in your cast
There was a problem hiding this comment.
I still don't really understand here, what exactly do you mean by "falls back to union scalar"? We already have a union scalar in the input
| } | ||
|
|
||
| /// Returns the non-null zero value for `dtype`, or [`None`] if no such value exists. | ||
| fn try_zero_value(dtype: &DType) -> Option<Self> { |
There was a problem hiding this comment.
I don't get why this needs to return option, Null is a weird type
There was a problem hiding this comment.
Null isn't the only type that returns None here though (see FSL, Struct, and now Union)? This just improves the error message in general
There was a problem hiding this comment.
because null returns none, I think the develop behaviour is better
There was a problem hiding this comment.
this is just an implementation detail though? this is a private helper so the behavior is the same (just different error message)?
| Self::Tuple(field_values) | ||
| } | ||
| DType::Union(..) => todo!("TODO(connor)[Union]: unimplemented"), | ||
| DType::Union(_) => vortex_panic!("{dtype} has no default value"), |
There was a problem hiding this comment.
this could pick first variant
There was a problem hiding this comment.
its not clear what to do if there are no variants (empty union / void type)
|
I think as_nullable for union has to traverse all the children and make them nullable. Since there's no top level nullability there's no other implementation that makes sense imho |
4893177 to
8ee294c
Compare
|
This does mean that |
|
why is it different? Struct has top level nullability while the union doesn't |
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
1e93238 to
bd74508
Compare
Rationale for this change
Depends on #8768.
Tracking Issue: #7882
UnionScalaris the row representation needed for scalar access and constant arrays before the canonicalUnionArray. Non-null values store the selected type ID and childScalarValue; null remains on the outerScalar, avoiding a second null layer.The DType/
UnionVariantsnullability propagation is split into #8768 so this PR contains only scalar-related changes.What changes are included in this PR?
UnionValue,UnionScalar, construction, downcasts, validation, display, equality, hashing, defaults, and sizingConstantArrayscalar access coverageScalar::into_nullablewith the Union nullability behavior from PropagateUnionnullability to its variants #8768What APIs are changed? Are there any user-facing changes?
Adds
ScalarValue::Union,UnionValue,UnionScalar,Scalar::union, and the Union scalar downcasts.Validation
cargo nextest run -p vortex-arraycargo clippy -p vortex-array --all-targets --all-featurescargo +nightly fmt --all -- --checkWorkspace-wide Clippy could not complete locally because the DuckDB build could not find
duckdb.h.