Add `UnionValue` and `UnionScalar` by connortsui20 · Pull Request #8838 · vortex-data/vortex

connortsui20 · 2026-07-17T21:53:42Z

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.

github-actions · 2026-07-17T21:56:32Z

Polar Signals Profiling Results

Latest Run

Status	Commit	Job	Attempt	Link
🟢 Done	`af8c64a`		1	Explore Profiling Data

Previous Runs (3)

Status	Commit	Attempt	Link
🟢 Done	`5e0113d`	1	Explore Profiling Data
🟢 Done	`fd2e580`	1	Explore Profiling Data
🟢 Done	`dd92c91`	1	Explore Profiling Data

Powered by Polar Signals Cloud

github-actions · 2026-07-17T21:57:30Z

Benchmarks: Vortex queries 📖

Verdict: No clear signal (low confidence)
Attributed Vortex impact: +4.5%
Engines: DataFusion No clear signal (+7.2%, medium confidence) · DuckDB No clear signal (+1.9%, low confidence)
Vortex (geomean): 1.009x ➖
Parquet (geomean): 0.987x ➖
Shifts: Parquet (control) -1.3% · Median polish +0.5%

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 (1.050x ➖, 0↑ 0↓)

name	PR `af8c64a` (ns)	base `ae9ecec` (ns)	ratio (PR/base)
vortex_q00/datafusion:vortex-file-compressed	9904614	9244952	1.07
vortex_q01/datafusion:vortex-file-compressed	6247013	6075932	1.03

datafusion / parquet (0.979x ➖, 0↑ 0↓)

name	PR `af8c64a` (ns)	base `ae9ecec` (ns)	ratio (PR/base)
vortex_q00/datafusion:parquet	20216752	20820621	0.97
vortex_q01/datafusion:parquet	4617876	4679197	0.99

duckdb / vortex-file-compressed (1.015x ➖, 0↑ 0↓)

name	PR `af8c64a` (ns)	base `ae9ecec` (ns)	ratio (PR/base)
vortex_q00/duckdb:vortex-file-compressed	10223004	10294945	0.99
vortex_q01/duckdb:vortex-file-compressed	6401161	6173707	1.04

duckdb / parquet (0.996x ➖, 0↑ 0↓)

name	PR `af8c64a` (ns)	base `ae9ecec` (ns)	ratio (PR/base)
vortex_q00/duckdb:parquet	23449091	23450715	1.00
vortex_q01/duckdb:parquet	9414912	9489168	0.99

No file size changes detected.

codspeed-hq · 2026-07-17T21:58:21Z

Merging this PR will regress 1 benchmark

⚡ 2 improved benchmarks
❌ 1 regressed benchmark
✅ 1785 untouched benchmarks
⏩ 54 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`new_raw_prim_test_between[i32, 2048]`	34 µs	38.8 µs	-12.37%
⚡	Simulation	`copy_nullable[65536]`	1.4 ms	1 ms	+31.81%
⚡	Simulation	`copy_non_nullable[65536]`	1,094.3 µs	913.5 µs	+19.8%

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 (af8c64a) with develop (ae9ecec)}

54 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. ↩

connortsui20 · 2026-07-17T22:28:19Z

+    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<_>>>()?
+        }


This was an existing "bug" (just how we display things), I just fixed it here.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

joseph-isaacs · 2026-07-20T14:28:55Z

-        match dtype {
-            DType::Null => vortex_panic!("Null dtype has no zero value"),
+        Self::try_zero_value(dtype)
+            .unwrap_or_else(|| vortex_panic!("{dtype} has no non-null zero value"))


This is odd?

why is this odd?

Call zero value on a scalar I would never expect to fail

we already panic here for null though?

I pointed this out on the other pr that this pr recreates that the change here is unnecessary noise. It's an internal helper function so why do we change it. We have added DType::Null for arrow compatibility and I think we shouldn't have

Given that we were considering vortex -> arrow union exporting via adding a null variant to the union to support our "outer null" conversion I think we have a good enough reason to keep it.

So it would be helpful for me if we decided if the semantics that I summarized in the PR description are the ones we want, rather than debating if the internal APIs that are affected by this are maybe unexpected (XY problem).

I thought that we had discussed this on Thursday and agreed that we want to have outer nulls and potentially add variant on export to arrow. I am commenting that I don't see how these two things are related and changes to this method seem unnecessary to me. Maybe I am missing something more fundamental here? If you removed the non throwing variant that would make more sense but yet again unless you do all of that the changes here are weird

lets try and meet tomorrow because I honestly do not understand what the issue is here, and there are other comments here that imply we are not all on the same page.

For this specific thing, is the issue the implementation or the internal API? I've seen the comments on how the API might be strange but, like I said above, we haven't actually talked about the thing that caused me to make that change, and so I don't understand understand the reasoning why this needs to be "fixed". It feels like we are talking past each other.

joseph-isaacs · 2026-07-20T14:29:09Z

                Self::Tuple(field_values)
            }
-            DType::Union(..) => todo!("TODO(connor)[Union]: unimplemented"),
+            DType::Union(..) => return None,


What do you want this to be

I dont understand this question. Are you asking about why this is None? I explain at the bottom of the PR description

## Rationale for this change - Closes: #8807 Three benchmarks stayed flaky after #8742, flipping between the same two values on PRs that can't affect them. Same root causes and fixes as #8742: | Benchmark | Seen flaky on | Why | Fix | | --- | --- | --- | --- | | `true_count_vortex_buffer[128]` | ±11.17% on 9 unrelated PRs (#8805, #8811, #8812, #8820, #8843, #8803, …) | a 128-bit popcount measures harness overhead and code layout, not the count | drop the 128 size | | runend `compress[(100000, 4)]` | ±11.9% on #8805, #8750, #8856 | allocates in the timed region; glibc malloc differs across runner images | mimalloc as global allocator | | `cast_decimal` `copy_*[65536]` | identical flags on #8838 and #8724 | same glibc-malloc cause (512 KB alloc per iteration) | mimalloc as global allocator | Left alone: `compact_sliced[(4096, 90)]` (single sighting) and the CUDA walltime benches (hosted-runner walltime noise, a runner config issue). The allocator swap shifts every benchmark in the two touched binaries once — see the comment below. Needs a one-time CodSpeed acknowledgment, like #8742. ## What changes are included in this PR? One commit per benchmark; bench files only. Ran `cargo check` + `clippy` on the three bench targets, smoke-ran the binaries, `cargo +nightly fmt`. --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>

yet Co-authored-by: Joe Isaacs <joe.isaacs@live.co.uk> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

connortsui20 changed the title ~~Add Union scalar support~~ Add UnionValue and UnionScalar Jul 17, 2026

connortsui20 added the changelog/feature A new feature label Jul 17, 2026

connortsui20 requested review from AdamGS, joseph-isaacs and robert3005 July 17, 2026 21:55

connortsui20 mentioned this pull request Jul 17, 2026

Preserve selected child dtype when formatting union scalars #8840

Closed

connortsui20 force-pushed the ct/union-scalar branch from dd92c91 to fd2e580 Compare July 17, 2026 22:18

connortsui20 commented Jul 17, 2026

View reviewed changes

Add Union scalar support

5e0113d

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

connortsui20 force-pushed the ct/union-scalar branch from fd2e580 to 5e0113d Compare July 17, 2026 22:51

joseph-isaacs reviewed Jul 20, 2026

View reviewed changes

connortsui20 requested a review from joseph-isaacs July 20, 2026 15:31

connortsui20 mentioned this pull request Jul 20, 2026

Fix remaining flaky CodSpeed microbenchmarks #8861

Merged

joseph-isaacs reviewed Jul 21, 2026

View reviewed changes

Comment thread vortex-array/src/scalar/cast.rs Outdated

joseph-isaacs approved these changes Jul 21, 2026

View reviewed changes

Update vortex-array/src/scalar/cast.rs

af8c64a

yet Co-authored-by: Joe Isaacs <joe.isaacs@live.co.uk> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

connortsui20 merged commit 85ed8e4 into develop Jul 21, 2026
82 of 85 checks passed

connortsui20 deleted the ct/union-scalar branch July 21, 2026 15:21

Uh oh!

Conversation

connortsui20 commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Inner Nulls with Type IDs

Value type

Casting

Default and Zero value semantics

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Polar Signals Profiling Results

Latest Run

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks: Vortex queries 📖

Uh oh!

codspeed-hq Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will regress 1 benchmark

Performance Changes

Footnotes

Uh oh!

connortsui20 Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

connortsui20 commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading

codspeed-hq Bot commented Jul 17, 2026 •

edited

Loading

connortsui20 Jul 17, 2026 •

edited

Loading