Skip to content

[Variant] Remove ceremony from iterator of variants into VariantArray #8625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 16, 2025
Merged

[Variant] Remove ceremony from iterator of variants into VariantArray #8625

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea264c4
Remove ceremony from iterator of variants into VariantArray
friendlymatthew Oct 15, 2025
48b8f0f
Give size hint to builder
friendlymatthew Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions parquet-variant-compute/src/variant_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

//! [`VariantArray`] implementation

use crate::VariantArrayBuilder;
use crate::type_conversion::{generic_conversion_single_value, primitive_conversion_single_value};
use arrow::array::{Array, ArrayRef, AsArray, BinaryViewArray, StructArray};
use arrow::buffer::NullBuffer;
Expand Down Expand Up @@ -439,6 +440,22 @@ impl From<VariantArray> for ArrayRef {
}
}

impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this:

Suggested change
impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
impl<'m, 'v, V: Into<Variant<'m, 'v>> FromIterator<Option<V>> for VariantArray {

then we can create a variant array from anything that converts cleanly to variant (e.g. i64 values or &str)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought about this as well, but I figured we'd be creating 2 abstractions?

As in, I wonder if it's better to guide the user to do the Variant::from explicitly 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

This is mostly going to be used by test code, and a homogenous set of e.g. i64 is a pretty common use case. But I don't have strong feelings either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed a ticket to track this idea

fn from_iter<T: IntoIterator<Item = Option<Variant<'m, 'v>>>>(iter: T) -> Self {
let iter = iter.into_iter();

let mut b = VariantArrayBuilder::new(iter.size_hint().0);
b.extend(iter);
b.build()
}
}

impl<'m, 'v> FromIterator<Variant<'m, 'v>> for VariantArray {
fn from_iter<T: IntoIterator<Item = Variant<'m, 'v>>>(iter: T) -> Self {
Self::from_iter(iter.into_iter().map(Some))
}
}

/// An iterator over [`VariantArray`]
///
/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
Expand Down Expand Up @@ -1141,6 +1158,7 @@ mod test {
use super::*;
use arrow::array::{BinaryViewArray, Int32Array};
use arrow_schema::{Field, Fields};
use parquet_variant::ShortString;

#[test]
fn invalid_not_a_struct_array() {
Expand Down Expand Up @@ -1405,4 +1423,48 @@ mod test {
assert!(i.next().is_none());
assert!(i.next_back().is_none());
}

#[test]
fn test_from_variant_opts_into_variant_array() {
let v = vec![None, Some(Variant::Null), Some(Variant::BooleanFalse), None];

let variant_array = VariantArray::from_iter(v);

assert_eq!(variant_array.len(), 4);

assert!(variant_array.is_null(0));

assert!(!variant_array.is_null(1));
assert_eq!(variant_array.value(1), Variant::Null);

assert!(!variant_array.is_null(2));
assert_eq!(variant_array.value(2), Variant::BooleanFalse);

assert!(variant_array.is_null(3));
}

#[test]
fn test_from_variants_into_variant_array() {
let v = vec![
Variant::Null,
Variant::BooleanFalse,
Variant::ShortString(ShortString::try_new("norm").unwrap()),
];

let variant_array = VariantArray::from_iter(v);

assert_eq!(variant_array.len(), 3);

assert!(!variant_array.is_null(0));
assert_eq!(variant_array.value(0), Variant::Null);

assert!(!variant_array.is_null(1));
assert_eq!(variant_array.value(1), Variant::BooleanFalse);

assert!(!variant_array.is_null(3));
assert_eq!(
variant_array.value(2),
Variant::ShortString(ShortString::try_new("norm").unwrap())
);
}
}
Loading