You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "friendlymatthew (via GitHub)" <gi...@apache.org> on 2025/10/15 14:19:50 UTC
[PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
friendlymatthew opened a new pull request, #8625:
URL: https://github.com/apache/arrow-rs/pull/8625
# Which issue does this PR close?
- Closes https://github.com/apache/arrow-rs/issues/8606
# Rationale for this change
Mapping from an iterator of `Variant`s into a `VariantArray` currently requires a lot of ceremony. This PR abstracts over this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#issuecomment-3406748448
> LGTM!
>
> > I'll throw in the other `impl From` in the second commit
>
> I'm not sure this is needed? `VariantArray::from_iterator` is pretty clear, and a `VariantArray::from` wouldn't be able to do anything smarter (can't just take ownership of the vec's data, for example)
Yeah, I agree. I'll just remove the second impl From
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2432819812
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -439,6 +440,22 @@ impl From<VariantArray> for ArrayRef {
}
}
+impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
+ fn from_iter<T: IntoIterator<Item = Option<Variant<'m, 'v>>>>(iter: T) -> Self {
+ let mut b = VariantArrayBuilder::new(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 {
+ let mut b = VariantArrayBuilder::new(0);
+ b.extend(iter);
+ b.build()
+ }
+}
+
Review Comment:
Most std lib code I've seen relies on the iterator's lower bound [hint](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint), with a fallback path if that turned out to be too low.
There's also the [ExactSizeIterator](https://doc.rust-lang.org/std/iter/trait.ExactSizeIterator.html) trait, but given how rust treats potentially overlapping generic definitions as conflicts, I don't know how to provide implementations for both Iterator and ExactSizeIterator?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2432793443
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -439,6 +440,22 @@ impl From<VariantArray> for ArrayRef {
}
}
+impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
+ fn from_iter<T: IntoIterator<Item = Option<Variant<'m, 'v>>>>(iter: T) -> Self {
+ let mut b = VariantArrayBuilder::new(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 {
+ let mut b = VariantArrayBuilder::new(0);
+ b.extend(iter);
+ b.build()
+ }
+}
+
Review Comment:
I wonder if there's a way to get the iterator size without consuming the iterator...
But then again, I haven't ran a profile on the `VariantArray` code yet. I plan on looking at perf later on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2433461938
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -439,6 +440,20 @@ impl From<VariantArray> for ArrayRef {
}
}
+impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
Review Comment:
Filed a ticket to track this idea
- https://github.com/apache/arrow-rs/issues/8628
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#issuecomment-3406690755
cc @alamb @scovich
Re: https://github.com/apache/arrow-rs/pull/8611#issuecomment-3405744977
I'll throw in the other `impl From` in the second commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2432778527
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -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.into_iter());
Review Comment:
Shouldn't need that `into_iter` call, the trait takes `IntoIterator` as input?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -439,6 +440,22 @@ impl From<VariantArray> for ArrayRef {
}
}
+impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
+ fn from_iter<T: IntoIterator<Item = Option<Variant<'m, 'v>>>>(iter: T) -> Self {
+ let mut b = VariantArrayBuilder::new(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 {
+ let mut b = VariantArrayBuilder::new(0);
+ b.extend(iter.into_iter().map(Some));
+ b.build()
Review Comment:
Would this work?
```suggestion
Self::from_iter(iter.into_iter().map(Some))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2433164448
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -439,6 +440,20 @@ impl From<VariantArray> for ArrayRef {
}
}
+impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2432795948
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -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.into_iter());
Review Comment:
Yeah I was just testing something and forgot to revert that. Luckily clippy caught it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2432836436
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -439,6 +440,20 @@ impl From<VariantArray> for ArrayRef {
}
}
+impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
Review Comment:
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 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Remove ceremony from iterator of variants into VariantArray [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8625:
URL: https://github.com/apache/arrow-rs/pull/8625#discussion_r2432827098
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -439,6 +440,20 @@ impl From<VariantArray> for ArrayRef {
}
}
+impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray {
Review Comment:
If we do this:
```suggestion
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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org