Skip to content

Part 2 of making Variant a canonical array#7143

Merged
AdamGS merged 2 commits intodevelopfrom
adamg/variant-canonical-2
Mar 24, 2026
Merged

Part 2 of making Variant a canonical array#7143
AdamGS merged 2 commits intodevelopfrom
adamg/variant-canonical-2

Conversation

@AdamGS
Copy link
Copy Markdown
Contributor

@AdamGS AdamGS commented Mar 24, 2026

Summary

Filling up missing parts of making Variant a canonical array. It still not fully supported but this is a step towards it.

These changes started as part of #7130, but I figured they are just noise in that already too big of a PR.

API Changes

Makes variant officially a canonical array and type.

@AdamGS AdamGS added feature A feature request changelog/feature A new feature and removed feature A feature request labels Mar 24, 2026
@AdamGS AdamGS requested a review from connortsui20 March 24, 2026 15:47
@AdamGS AdamGS assigned gatesn and unassigned gatesn Mar 24, 2026
@AdamGS AdamGS requested a review from gatesn March 24, 2026 15:48
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
@AdamGS AdamGS force-pushed the adamg/variant-canonical-2 branch from b2879b8 to 5a25819 Compare March 24, 2026 15:48
Canonical::Extension(a) => {
Canonical::Extension(mask_validity_extension(a, validity_mask, ctx)?)
}
Canonical::Variant(_) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it not worth doing the push-down now?

Canonical::Extension(ExtensionArray::new(a.ext_dtype().clone(), filtered_storage))
}
Canonical::Variant(_) => {
vortex_panic!("Variant arrays don't support filtering")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it not worth doing the push-down now?

Canonical::Struct(a) => Canonical::Struct(take_struct(&a, codes)),
Canonical::Extension(a) => Canonical::Extension(take_extension(&a, codes, ctx)),
Canonical::Variant(_) => {
vortex_bail!("Variant arrays don't support Take")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it not worth doing the push-down now?

@AdamGS
Copy link
Copy Markdown
Contributor Author

AdamGS commented Mar 24, 2026

re pushdown - mostly figured I'll do it as part of/after the next PR, so I have a more concrete thing to test this end to end.

Signed-off-by: Adam Gutglick <adam@spiraldb.com>
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 24, 2026

Merging this PR will degrade performance by 15.51%

❌ 2 regressed benchmarks
✅ 1099 untouched benchmarks
⏩ 1522 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation bitwise_not_vortex_buffer_mut[128] 317.8 ns 376.1 ns -15.51%
Simulation bitwise_not_vortex_buffer_mut[1024] 477.2 ns 535.6 ns -10.89%

Comparing adamg/variant-canonical-2 (22ada74) with develop (cda251b)

Open in CodSpeed

Footnotes

  1. 1522 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.

@AdamGS AdamGS merged commit 0ea8973 into develop Mar 24, 2026
59 of 60 checks passed
@AdamGS AdamGS deleted the adamg/variant-canonical-2 branch March 24, 2026 17:13
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.

2 participants