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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
