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 14:16:06 UTC
[I] [Variant] Remove potential panics when probing `VariantArray` [arrow-rs]
friendlymatthew opened a new issue, #8609:
URL: https://github.com/apache/arrow-rs/issues/8609
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
Consider the following:
```rs
fn foo() {
let mut b = VariantArrayBuilder::new(1);
b.append_null();
let v = b.build();
v.value(0); // will panic
}
```
Probing a `VariantArray` when that element is null will result in a panic:
```rs
thread 'cast_to_variant::tests::foo' panicked at /Users/matthewkim/.cargo/git/checkouts/arrow-rs-583cca34693b79b8/760b7b6/parquet-variant/src/variant.rs:318:14:
Invalid variant metadata: InvalidArgumentError("Received empty bytes")
```
This seems like an odd pattern, as it's not clear exactly what a user should do when iterating or probing through a `VariantArray`
**Describe the solution you'd like**
We could have `VariantArray::value(i: usize)` return an `Option<Variant<'a>>`. This way, it's clear when it's `None`, that element is considered `null`. _Note, this is different from `Some(Variant::Null))`_
**Additional context**
<!--
Add any other context or screenshots about the feature request here.
-->
This feature is motivated by [datafusion-variant](https://github.com/datafusion-contrib/datafusion-variant), a higher-level library that integrates with Datafusion through a series of udfs
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Variant] Remove potential panics when probing `VariantArray` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on issue #8609:
URL: https://github.com/apache/arrow-rs/issues/8609#issuecomment-3403173750
I think you've identified two separate issues:
> Probing a `VariantArray` when that element is null will result in a panic
This is arguably a bug that needs fixing, and the immediate fix is likely a one-liner:
<details>
```diff
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match (self.typed_value_field(), self.value_field()) {
// Always prefer typed_value, if available
(Some(typed_value), value) if typed_value.is_valid(index) => {
typed_value_to_variant(typed_value, value, index)
}
// Otherwise fall back to value, if available
- (_, Some(value)) if value.is_valid(index) => {
+ (_, Some(value)) if self.metadata.is_valid(index) && value.is_valid(index) => {
Variant::new(self.metadata.value(index), value.value(index))
}
// It is technically invalid for neither value nor typed_value fields to be available,
// but the spec specifically requires readers to return Variant::Null in this case.
_ => Variant::Null,
}
```
</details>
... but only because `VariantArray::typed_value_to_variant` doesn't support partially shredded objects yet.
<details>
Once we support those, that helper will need to take the `metadata` column as an additional arg, and the lifetime annotations will likely need to change as well. Actually... I think the method _cannot_ support partially shredded objects because `Variant` is fundamentally a reference type, and returning a single `Variant` for a partially shredded object would require reassembling it. Or adding the new `Variant::ShreddedObject` enum variant that @alamb has mentioned a couple of times as a way to capture all the raw bytes as-is, without trying to reassemble them.
If we always unshred all primitive types, then it _might_ be as simple as adding new enum variants `ShreddedObject(ShreddedVariantObject)` and `ShreddedList(Vec<Variant<'m, 'v>)` where:
```rust
struct ShreddedVariantObject<'m, 'v> {
unshredded_fields: Option<VariantObject<'m, 'v>>,
shredded_fields: Vec<(&'m str, Variant<'m, 'v>)>,
}
```
The hard part is _populating_ it, not defining it...
</details>
Meanwhile, there are some other panics lurking in that method, which we should probably track fixes for as well?
> This seems like an odd pattern, as it's not clear exactly what a user should do when iterating or probing through a `VariantArray`
For any arrow array, a caller who cares about nulls needs to check `Array::is_null` or `Array::is_valid` before invoking the `value` method. A blind `v.value(0)` is never recommended (but it still shouldn't panic).
--
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: [I] [Variant] Remove potential panics when probing `VariantArray` [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on issue #8609:
URL: https://github.com/apache/arrow-rs/issues/8609#issuecomment-3402393000
This would be solved via http://github.com/apache/arrow-rs/issues/8612
I noticed the solution I described isn't necessarily idiomatic to the array convention. `Option<T>` is usually delegated to `iter()` calls on the array itself
--
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: [I] [Variant] Remove potential panics when probing `VariantArray` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #8609:
URL: https://github.com/apache/arrow-rs/issues/8609#issuecomment-3416684545
`label_issue.py` automatically added labels {'parquet-variant'} from #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: [I] [Variant] Remove potential panics when probing `VariantArray` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #8609: [Variant] Remove potential panics when probing `VariantArray`
URL: https://github.com/apache/arrow-rs/issues/8609
--
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