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/14 16:54:44 UTC
[PR] [Variant] Impl `arrow::Array` for `VariantArray` [arrow-rs]
friendlymatthew opened a new pull request, #8613:
URL: https://github.com/apache/arrow-rs/pull/8613
# Which issue does this PR close?
- Closes https://github.com/apache/arrow-rs/issues/8612
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#issuecomment-3403338127
cc @scovich
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430721093
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
Review Comment:
Agree the `inspect` call was too clever, but why would this hurt readability?
```rust
(self.head_i < self.tail_i).then(|| {
let out = self.value_opt(self.head_i);
self.head_i += 1;
out
})
```
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#issuecomment-3402988605
> For context this was removed in #8392
Thanks
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430320020
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -436,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
Review Comment:
thank you for the comments
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -436,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
+ }
+
+ fn size_hint(&self) -> (usize, Option<usize>) {
Review Comment:
I double checked the definition of size_hint and this looks good:
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430992735
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
Review Comment:
I think cases like the following are completely valid and very readable. It's simply inspecting the state
```rs
(self.head_i < self.tail_i).then(|| self.value_opt(self.head_i));
```
In the closure you posted above, I'm just a bit surprised as a reader that the closure is inspecting the state _and_ mutating it. Plus, I think the following form is simpler to reason through
```rs
// base case
if self.head_i == self.tail_i {
return None;
}
// else
let out = self.value_opt(self.head_i);
self.head_i += 1;
Some(out)
```
But then again, it's just personal preference. I don't think there's anything wrong with either approach
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430681072
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
Review Comment:
Hi hi, I think using combinators are great, but in this case we're 1) hurting readability and 2) using `Option::inspect` to mutate state
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
Review Comment:
I like the idea of `value_opt`!
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430681072
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
Review Comment:
Hi hi, I think using combinators are great, but in this case we're 1) hurting readability and 2) using `Option::inspect` to mutate state
I find using combinators on a bool term to be more confusing than a simple if branch, but this is just my opinion
--
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] Impl `arrow::Array` for `VariantArray` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#issuecomment-3402856668
For context this was removed in https://github.com/apache/arrow-rs/pull/8392
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430351410
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
Review Comment:
`is_valid` would probably be more useful, see other comment below
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
Review Comment:
```suggestion
(self.head_i < self.tail_i)
.then(|| self.value_opt(self.head_i))
.inspect(|_| self.head_i += 1)
```
(same story for `DoubleEndedIterator` below)
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
Review Comment:
But actually -- Right now we surface an `is_null` or `is_valid` that has multiple call sites who must each also call `self.array.value`. Why not wrap the whole operation instead?
```rust
fn value_opt(&self, idx: usize) -> Option<Self::Item> {
self.array.is_valid(idx).then(|| self.array.value(idx))
}
```
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#issuecomment-3405802312
Thanks @scovich and @friendlymatthew
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430992735
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
Review Comment:
I think cases like the following are completely valid and very readable. It's simply inspecting the state
```rs
(self.head_i < self.tail_i).then(|| self.value_opt(self.head_i));
```
In the closure you posted above, I'm just a bit surprised as a reader that the closure is inspecting the state _and_ mutating it. Plus, I think the following form is simpler to reason through
```rs
// base case
if self.head_i == self.tail_i {
return None;
}
// else
let out = self.value_opt(self.head_i);
self.head_i += 1;
return out
```
But then again, it's just personal preference
--
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] Make `VariantArray` iterable [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8613:
URL: https://github.com/apache/arrow-rs/pull/8613#discussion_r2430992735
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -434,6 +441,99 @@ impl From<VariantArray> for ArrayRef {
}
}
+/// An iterator over [`VariantArray`]
+///
+/// This iterator returns `Option<Option<Variant<'a, 'a>>>` where:
+/// - `None` indicates the end of iteration
+/// - `Some(None)` indicates a null value at this position
+/// - `Some(Some(variant))` indicates a valid variant value
+///
+/// # Example
+///
+/// ```
+/// # use parquet_variant::Variant;
+/// # use parquet_variant_compute::VariantArrayBuilder;
+/// let mut builder = VariantArrayBuilder::new(10);
+/// builder.append_variant(Variant::from(42));
+/// builder.append_null();
+/// builder.append_variant(Variant::from("hello"));
+/// let array = builder.build();
+///
+/// let values = array.iter().collect::<Vec<_>>();
+/// assert_eq!(values.len(), 3);
+/// assert_eq!(values[0], Some(Variant::from(42)));
+/// assert_eq!(values[1], None);
+/// assert_eq!(values[2], Some(Variant::from("hello")));
+/// ```
+#[derive(Debug)]
+pub struct VariantArrayIter<'a> {
+ array: &'a VariantArray,
+ head_i: usize,
+ tail_i: usize,
+}
+
+impl<'a> VariantArrayIter<'a> {
+ /// Creates a new iterator over the given [`VariantArray`]
+ pub fn new(array: &'a VariantArray) -> Self {
+ Self {
+ array,
+ head_i: 0,
+ tail_i: array.len(),
+ }
+ }
+
+ /// Helper method to check if the value at the given index is null
+ #[inline]
+ fn is_null(&self, idx: usize) -> bool {
+ self.array.is_null(idx)
+ }
+}
+
+impl<'a> Iterator for VariantArrayIter<'a> {
+ type Item = Option<Variant<'a, 'a>>;
+
+ #[inline]
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.head_i == self.tail_i {
+ return None;
+ }
+
+ let out = if self.is_null(self.head_i) {
+ Some(None)
+ } else {
+ Some(Some(self.array.value(self.head_i)))
+ };
+
+ self.head_i += 1;
+
+ out
Review Comment:
I think cases like the following are completely valid and very readable. It's simply inspecting the state
```rs
(self.head_i < self.tail_i).then(|| self.value_opt(self.head_i));
```
In the closure you posted above, I'm just a bit surprised as a reader that the closure is inspecting the state _and_ mutating it. Plus, I think the following form is simpler to reason through
```rs
// base case
if self.head_i == self.tail_i {
return None;
}
// else
let out = self.value_opt(self.head_i);
self.head_i += 1;
Some(out)
```
But then again, it's just personal preference
--
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