Add UnionValue and UnionScalar#8838
Conversation
Polar Signals Profiling ResultsLatest Run
Previous Runs (2)
Powered by Polar Signals Cloud |
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.861x ✅, 2↑ 0↓)
datafusion / parquet (0.834x ✅, 2↑ 0↓)
duckdb / vortex-file-compressed (0.985x ➖, 0↑ 0↓)
duckdb / parquet (0.960x ➖, 0↑ 0↓)
No file size changes detected. |
Merging this PR will degrade performance by 17.18%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
dd92c91 to
fd2e580
Compare
| let values = match dtype { | ||
| DType::List(element_dtype, _) | DType::FixedSizeList(element_dtype, ..) => v | ||
| .values | ||
| .iter() | ||
| .map(|elem| ScalarValue::from_proto(elem, element_dtype.as_ref(), session)) | ||
| .collect::<VortexResult<Vec<_>>>()?, | ||
| DType::Struct(fields, _) => { | ||
| vortex_ensure_eq!( | ||
| v.values.len(), fields.nfields(), | ||
| Serde: "expected {} struct fields in ListValue, got {}", | ||
| fields.nfields(), | ||
| v.values.len() | ||
| ); | ||
|
|
||
| v.values | ||
| .iter() | ||
| .zip(fields.fields()) | ||
| .map(|(value, field_dtype)| ScalarValue::from_proto(value, &field_dtype, session)) | ||
| .collect::<VortexResult<Vec<_>>>()? | ||
| } |
There was a problem hiding this comment.
This was an existing bug, I just fixed it here.
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
fd2e580 to
5e0113d
Compare
Revived from #8791 since too many things have changed.
Summary
Tracking issue: #8769
Adds scalar implementation for
Uniontype in Vortex.Changes
Adds
UnionValuewhich exists inside ofScalarValueand the typed viewUnionScalar<'a>.Inner Nulls with Type IDs
The type above implies that type IDs MATTER for equality of nulls. 2 values with the same union type might both be null, but if the type IDs are different, we consider them NOT equal. Please respond below if you think this is the incorrect behavior. I think that this might be different from arrow's semantics, but because we are already distinguishing from them with the outer nullability, I think this is fine.
Value type
I was debating if we should instead store aOption<Box<ScalarValue>>instead of justBox<Scalar>(which carries an extra dtype and an extra allocation even if the value is null). I think it is nice to have the dtype in there for implementation ease. Not sure if it's worth it.We use
Option<Box<ScalarValue>>instead ofBox<Scalar>so that we do not storeDTypein multiple places. If we did so, we run into synchronization issues with casting.Casting
For casting, this only allows identity and null casting (like everything else), otherwise it just panics with unimplemented. We probably want to figure this out later as it is quite valuable to cast union types.
Default and Zero value semantics
Finally, the "default value" and "zero value" semantics here are as such:
default_value(nullable Union)-> returns an outer null.default_value(non-nullable Union)-> panics because there is no designated default variant.zero_value(Union)-> panics because a Union has no canonical zero variant.is_zero()on a null Union -> returnsNone.is_zero()on a valid Union -> returnsSome(false), even when its child is numerically zero.Note that none of these behaviors depend on variant ordering. And also I do think that it is possible that we just choose the first variant of a union to be the default, I think this is kind of opinionated, and it also doesn't solve the issue where there are 0 variants (empty union / void type). So I would prefer if we do this in a separate change.