Skip to content

Add UnionValue and UnionScalar#8838

Open
connortsui20 wants to merge 1 commit into
developfrom
ct/union-scalar
Open

Add UnionValue and UnionScalar#8838
connortsui20 wants to merge 1 commit into
developfrom
ct/union-scalar

Conversation

@connortsui20

@connortsui20 connortsui20 commented Jul 17, 2026

Copy link
Copy Markdown
Member

Revived from #8791 since too many things have changed.

Summary

Tracking issue: #8769

Adds scalar implementation for Union type in Vortex.

Changes

Adds UnionValue which exists inside of ScalarValue and the typed view UnionScalar<'a>.

pub struct UnionValue {
    /// The type ID selecting a variant in the enclosing [`DType::Union`].
    type_id: u8,

    /// The selected variant scalar.
    ///
    /// This is boxed to break the recursive layout between [`ScalarValue`]
    /// and [`UnionValue`].
    value: `Option<Box<ScalarValue>>`,
}

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 a Option<Box<ScalarValue>> instead of just Box<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 of Box<Scalar> so that we do not store DType in 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 -> returns None.
  • is_zero() on a valid Union -> returns Some(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.

@connortsui20 connortsui20 changed the title Add Union scalar support Add UnionValue and UnionScalar Jul 17, 2026
@connortsui20 connortsui20 added the changelog/feature A new feature label Jul 17, 2026
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Polar Signals Profiling Results

Latest Run

Status Commit Job Attempt Link
🟢 Done 5e0113d 1 Explore Profiling Data
Previous Runs (2)
Status Commit Job Attempt Link
🟢 Done fd2e580 1 Explore Profiling Data
🟢 Done dd92c91 1 Explore Profiling Data

Powered by Polar Signals Cloud

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Benchmarks: Vortex queries 📖

Verdict: No clear signal (low confidence)
Attributed Vortex impact: +3.0%
Engines: DataFusion No clear signal (+3.2%, low confidence) · DuckDB No clear signal (+2.7%, low confidence)
Vortex (geomean): 0.908x ➖
Parquet (geomean): 0.894x ✅
Shifts: Parquet (control) -10.6% · Median polish -9.1%

How to read Verdict and Engines
  • Verdict: Overall PR-level signal after subtracting baseline drift estimated from Parquet control rows. It can be Likely improvement, Likely regression, or No clear signal.
  • Engines: Per-engine attribution. DataFusion is compared against DataFusion/Parquet controls; DuckDB is compared against DuckDB/Parquet controls. This answers whether each engine improved or regressed independently.
  • Confidence: Based on directional consistency, share of rows above the noise floor, and control-run noise.

datafusion / vortex-file-compressed (0.861x ✅, 2↑ 0↓)
name PR 5e0113d (ns) base e6eeaae (ns) ratio (PR/base)
vortex_q00/datafusion:vortex-file-compressed 🚀 10090931 11446321 0.88
vortex_q01/datafusion:vortex-file-compressed 🚀 6258939 7449028 0.84
datafusion / parquet (0.834x ✅, 2↑ 0↓)
name PR 5e0113d (ns) base e6eeaae (ns) ratio (PR/base)
vortex_q00/datafusion:parquet 🚀 19857688 23584739 0.84
vortex_q01/datafusion:parquet 🚀 4635116 5616200 0.83
duckdb / vortex-file-compressed (0.985x ➖, 0↑ 0↓)
name PR 5e0113d (ns) base e6eeaae (ns) ratio (PR/base)
vortex_q00/duckdb:vortex-file-compressed 9940192 10387695 0.96
vortex_q01/duckdb:vortex-file-compressed 5972523 5885688 1.01
duckdb / parquet (0.960x ➖, 0↑ 0↓)
name PR 5e0113d (ns) base e6eeaae (ns) ratio (PR/base)
vortex_q00/duckdb:parquet 23375349 24576114 0.95
vortex_q01/duckdb:parquet 9415453 9723548 0.97

No file size changes detected.

@codspeed-hq

codspeed-hq Bot commented Jul 17, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 17.18%

❌ 3 regressed benchmarks
✅ 1667 untouched benchmarks
⏩ 53 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation copy_nullable[65536] 1 ms 1.4 ms -24.26%
Simulation copy_non_nullable[65536] 908.8 µs 1,089.5 µs -16.59%
Simulation compact_sliced[(4096, 90)] 780.3 ns 867.8 ns -10.08%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/union-scalar (5e0113d) with develop (e6eeaae)

Open in CodSpeed

Footnotes

  1. 53 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Comment on lines +454 to +473
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<_>>>()?
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an existing bug, I just fixed it here.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant