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