You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "carpecodeum (via GitHub)" <gi...@apache.org> on 2025/08/18 18:51:00 UTC
[PR] Support Shredded Objects in variant_get [arrow-rs]
carpecodeum opened a new pull request, #8166:
URL: https://github.com/apache/arrow-rs/pull/8166
# Which issue does this PR close?
- Closes #8150
# Rationale for this change
Support variant_get for any input (shredded or otherwise), any depth of object field path steps, and casting to one primitive data type, eg. Some(DataType::Int32). This is build on top of changes in https://github.com/apache/arrow-rs/pull/8122
# What changes are included in this PR?
Add support for extracting fields from both shredded and non-shredded variant arrays at any depth (like "x", "a.x", "a.b.x") and casting them to Int32 with proper NULL handling for type mismatches.
# Are these changes tested?
Yes, tests are added for non-shredded vs shredded inputs at depths 0, 1, and 2
# Are there any user-facing changes?
not yet
Thanks to @mprammer 🥷
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3256422729
> > @carpecodeum , @scovich points out that this PR now also has merge conflicts, likely due to merging this
> >
> > * [[Variant] Support typed access for numeric types in variant_get #8179](https://github.com/apache/arrow-rs/pull/8179)
> >
> > > the rebase will have to figure out how to move the newly merged output builder functionality to an appropriate row builder equivalent.
> >
> >
> > Would you like help doing this? I can make another PR (targeting your branch to do so).
>
> Hi @alamb @scovich My apologies, I had been quite busy since the past week, I would like to help with the merge conflict resolution, I realise with so many changes going in right now, stuff has become hard to track, including with this PR as well
I had some boring meetings where I could supervise a surprisingly helpful AI assistant. PTAL?
* https://github.com/apache/arrow-rs/pull/8280
(I tried making https://github.com/cmu-db/arrow-rs/pull/5 against your branch, but I must have done it completely wrong... either that, or the merge commit threw things off; maybe somebody smarter than me can figure that one 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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2286231749
##########
parquet-variant-compute/src/variant_get/output/mod.rs:
##########
@@ -33,6 +34,7 @@ use arrow_schema::{ArrowError, DataType};
/// or as a specific type (e.g. Int32Array).
///
/// See [`instantiate_output_builder`] to create an instance of this trait.
+#[allow(unused)]
pub(crate) trait OutputBuilder {
Review Comment:
Are we planning to just delete output builder-related code at some point?
(tho I guess that could be a follow-on PR?)
##########
parquet-variant/src/path.rs:
##########
@@ -95,10 +95,19 @@ impl<'a> From<Vec<VariantPathElement<'a>>> for VariantPath<'a> {
}
}
-/// Create from &str
+/// Create from &str with support for dot notation
impl<'a> From<&'a str> for VariantPath<'a> {
fn from(path: &'a str) -> Self {
- VariantPath::new(vec![path.into()])
+ // Support dot notation: "a.x" -> ["a", "x"]
+ if path.contains('.') {
+ let elements: Vec<VariantPathElement<'a>> = path
+ .split('.')
+ .map(|part| part.into())
+ .collect();
+ VariantPath::new(elements)
Review Comment:
nit: I'm guessing the type annotation is unnecessary, because `VariantPath::new` already constrains it to a vec of path elements?
```suggestion
VariantPath::new(path.split('.').map(Into::into).collect())
```
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -192,7 +202,101 @@ impl VariantArray {
/// Return a reference to the metadata field of the [`StructArray`]
pub fn metadata_field(&self) -> &BinaryViewArray {
- self.shredding_state.metadata_field()
+ &self.metadata
+ }
+
+ /// Return a reference to the value field of the `StructArray`
+ pub fn value_field(&self) -> Option<&BinaryViewArray> {
+ self.shredding_state.value_field()
+ }
+
+ /// Return a reference to the typed_value field of the `StructArray`, if present
+ pub fn typed_value_field(&self) -> Option<&ArrayRef> {
+ self.shredding_state.typed_value_field()
+ }
+}
+
+/// One shredded field of a partially or prefectly shredded variant. For example, suppose the
+/// shredding schema for variant `v` treats it as an object with a single field `a`, where `a` is
+/// itself a struct with the single field `b` of type INT. Then the physical layout of the column
+/// is:
+///
+/// ```text
+/// v: VARIANT {
+/// metadata: BINARY,
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
+/// value: BINARY,
+/// typed_value: INT,
+/// },
+/// },
+/// },
+/// },
+/// }
+/// ```
+///
+/// In the above, each row of `v.value` is either a variant value (shredding failed, `v` was not an
+/// object at all) or a variant object (partial shredding, `v` was an object but included unexpected
+/// fields other than `a`), or is NULL (perfect shredding, `v` was an object containing only the
+/// single expected field `a`).
+///
+/// A similar story unfolds for each `v.typed_value.a.value` -- a variant value if shredding failed
+/// (`v:a` was not an object at all), or a variant object (`v:a` was an object with unexpected
+/// additional fields), or NULL (`v:a` was an object containing only the single expected field `b`).
+///
+/// Finally, `v.typed_value.a.typed_value.b.value` is either NULL (`v:a.b` was an integer) or else a
+/// variant value.
Review Comment:
```suggestion
/// variant value (which could be `Variant::Null`).
```
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -166,23 +180,19 @@ impl VariantArray {
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {
- ShreddingState::Unshredded { metadata, value } => {
- Variant::new(metadata.value(index), value.value(index))
+ ShreddingState::Unshredded { value } => {
Review Comment:
NOTE: This change will conflict with
* https://github.com/apache/arrow-rs/pull/8093
We may want to be intentional about which PR merges first? (AFAIK, this PR should "just work" even if the other PR merges later... but it's probably more convenient to let the other PR merge first).
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -230,107 +390,138 @@ pub enum ShreddingState {
// https://github.com/apache/arrow-rs/issues/8088
// Missing { metadata: BinaryViewArray },
/// This variant has no typed_value field
- Unshredded {
- metadata: BinaryViewArray,
- value: BinaryViewArray,
- },
+ Unshredded { value: BinaryViewArray },
/// This variant has a typed_value field and no value field
/// meaning it is the shredded type
- Typed {
- metadata: BinaryViewArray,
- typed_value: ArrayRef,
- },
- /// Partially shredded:
- /// * value is an object
- /// * typed_value is a shredded object.
+ PerfectlyShredded { typed_value: ArrayRef },
+ /// Imperfectly shredded: Shredded values reside in `typed_value` while those that failed to
+ /// shred reside in `value`. Missing field values are NULL in both columns, while NULL primitive
+ /// values have NULL `typed_value` and `Variant::Null` in `value`.
///
- /// Note the spec says "Writers must not produce data where both value and
- /// typed_value are non-null, unless the Variant value is an object."
- PartiallyShredded {
- metadata: BinaryViewArray,
+ /// NOTE: A partially shredded struct is a special kind of imperfect shredding, where
+ /// `typed_value` and `value` are both non-NULL. The `typed_value` is a struct containing the
+ /// subset of fields for which shredding was attempted (each field will then have its own value
+ /// and/or typed_value sub-fields that indicate how shredding actually turned out). Meanwhile,
+ /// the `value` is a variant object containing the subset of fields for which shredding was
+ /// not even attempted.
+ ImperfectlyShredded {
value: BinaryViewArray,
typed_value: ArrayRef,
},
}
impl ShreddingState {
/// try to create a new `ShreddingState` from the given fields
- pub fn try_new(
- metadata: BinaryViewArray,
- value: Option<BinaryViewArray>,
- typed_value: Option<ArrayRef>,
- ) -> Result<Self, ArrowError> {
- match (metadata, value, typed_value) {
- (metadata, Some(value), Some(typed_value)) => Ok(Self::PartiallyShredded {
- metadata,
- value,
- typed_value,
- }),
- (metadata, Some(value), None) => Ok(Self::Unshredded { metadata, value }),
- (metadata, None, Some(typed_value)) => Ok(Self::Typed {
- metadata,
- typed_value,
- }),
- (_metadata_field, None, None) => Err(ArrowError::InvalidArgumentError(String::from(
+ pub fn try_new(inner: &StructArray) -> Result<Self, ArrowError> {
+ // Note the specification allows for any order so we must search by name
+
+ // Find the value field, if present
+ let value = inner
+ .column_by_name("value")
+ .map(|v| {
+ v.as_binary_view_opt().ok_or_else(|| {
+ ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ v.data_type()
+ ))
+ })
+ })
+ .transpose()?
+ .cloned();
+
+ // Find the typed_value field, if present
+ let typed_value = inner.column_by_name("typed_value").cloned();
+
+ match (value, typed_value) {
+ (Some(value), Some(typed_value)) => {
+ Ok(Self::ImperfectlyShredded { value, typed_value })
+ }
+ (Some(value), None) => Ok(Self::Unshredded { value }),
+ (None, Some(typed_value)) => Ok(Self::PerfectlyShredded { typed_value }),
+ (None, None) => Err(ArrowError::InvalidArgumentError(String::from(
"VariantArray has neither value nor typed_value field",
))),
}
}
- /// Return a reference to the metadata field
- pub fn metadata_field(&self) -> &BinaryViewArray {
- match self {
- ShreddingState::Unshredded { metadata, .. } => metadata,
- ShreddingState::Typed { metadata, .. } => metadata,
- ShreddingState::PartiallyShredded { metadata, .. } => metadata,
- }
- }
-
/// Return a reference to the value field, if present
pub fn value_field(&self) -> Option<&BinaryViewArray> {
match self {
ShreddingState::Unshredded { value, .. } => Some(value),
- ShreddingState::Typed { .. } => None,
- ShreddingState::PartiallyShredded { value, .. } => Some(value),
+ ShreddingState::PerfectlyShredded { .. } => None,
+ ShreddingState::ImperfectlyShredded { value, .. } => Some(value),
}
}
/// Return a reference to the typed_value field, if present
pub fn typed_value_field(&self) -> Option<&ArrayRef> {
match self {
ShreddingState::Unshredded { .. } => None,
- ShreddingState::Typed { typed_value, .. } => Some(typed_value),
- ShreddingState::PartiallyShredded { typed_value, .. } => Some(typed_value),
+ ShreddingState::PerfectlyShredded { typed_value, .. } => Some(typed_value),
+ ShreddingState::ImperfectlyShredded { typed_value, .. } => Some(typed_value),
}
}
/// Slice all the underlying arrays
pub fn slice(&self, offset: usize, length: usize) -> Self {
match self {
- ShreddingState::Unshredded { metadata, value } => ShreddingState::Unshredded {
- metadata: metadata.slice(offset, length),
- value: value.slice(offset, length),
- },
- ShreddingState::Typed {
- metadata,
- typed_value,
- } => ShreddingState::Typed {
- metadata: metadata.slice(offset, length),
- typed_value: typed_value.slice(offset, length),
- },
- ShreddingState::PartiallyShredded {
- metadata,
- value,
- typed_value,
- } => ShreddingState::PartiallyShredded {
- metadata: metadata.slice(offset, length),
+ ShreddingState::Unshredded { value } => ShreddingState::Unshredded {
Review Comment:
Another place where the enum variants just add complexity?
```rust
Self {
value: self.value.map(|v| v.slice(offset_length)),
typed_value: self.typed_value.map(|tv| tv.slice(offset, length)),
}
```
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
Review Comment:
```suggestion
/// The path element is not present in the `typed_value` column and must be retrieved from the `value`
```
?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ if shredding_state.value_field().is_none() {
+ ShreddedPathStep::Missing
+ } else {
+ ShreddedPathStep::NotShredded
+ }
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ let Some(field) = typed_value
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .and_then(|typed_value| typed_value.column_by_name(name))
+ else {
+ return Ok(missing_path_step());
+ };
+
+ let field = field
+ .as_any()
+ .downcast_ref::<ShreddedVariantFieldArray>()
+ .ok_or_else(|| {
+ // TODO: Should we blow up? Or just end the traversal and let the normal
+ // variant pathing code sort out the mess that it must anyway be
+ // prepared to handle?
+ ArrowError::InvalidArgumentError(format!(
+ "Expected a ShreddedVariantFieldArray, got {:?} instead",
+ field.data_type(),
+ ))
+ })?;
+
+ Ok(ShreddedPathStep::Success(field.shredding_state()))
+ }
+ VariantPathElement::Index { .. } => {
+ // TODO: Support array indexing. Among other things, it will require slicing not
+ // only the array we have here, but also the corresponding metadata and null masks.
+ Err(ArrowError::NotYetImplemented(
+ "Pathing into shredded variant array index".into(),
+ ))
+ }
+ }
+}
+
+/// Follows the given path as far as possible through shredded variant fields. If the path ends on a
+/// shredded field, return it directly. Otherwise, use a row shredder to follow the rest of the path
+/// and extract the requested value on a per-row basis.
+fn shredded_get_path(
+ input: &VariantArray,
+ path: &[VariantPathElement<'_>],
+ as_type: Option<&DataType>,
+) -> Result<ArrayRef> {
+ // Helper that creates a new VariantArray from the given nested value and typed_value columns,
+ let make_target_variant = |value: Option<BinaryViewArray>, typed_value: Option<ArrayRef>| {
+ let metadata = input.metadata_field().clone();
+ let nulls = input.inner().nulls().cloned();
+ VariantArray::from_parts(
+ metadata,
+ value,
+ typed_value,
+ nulls,
Review Comment:
This is incorrect, or at least incomplete -- we need to [union](https://docs.rs/arrow/latest/arrow/buffer/struct.NullBuffer.html#method.union) the null buffers of all shredded ancestors we pathed through to reach this point. E.g. if the path was `v:a.b.c::INT`, with `v` itself nullable, then the null mask calculation is something like:
```rust
let nulls = v.nulls()
| v.typed_value.nulls()
| v.typed_value.a.typed_value.nulls()
| v.typed_value.a.typed_value.b.typed_value.nulls()
| v.typed_value.a.typed_value.b.typed_value.c.logical_nulls();
```
(the above assumes that `c.logical_nulls()` computes the intersection of `c.typed_value` and `c.value` columns, since `c` itself is physically non-nullable but can still encode logical nulls as both-children-null)
In reality, most of the unioning should happen as part of the path step loop.
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ if shredding_state.value_field().is_none() {
+ ShreddedPathStep::Missing
+ } else {
+ ShreddedPathStep::NotShredded
+ }
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ let Some(field) = typed_value
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .and_then(|typed_value| typed_value.column_by_name(name))
+ else {
+ return Ok(missing_path_step());
+ };
+
+ let field = field
+ .as_any()
+ .downcast_ref::<ShreddedVariantFieldArray>()
+ .ok_or_else(|| {
+ // TODO: Should we blow up? Or just end the traversal and let the normal
+ // variant pathing code sort out the mess that it must anyway be
+ // prepared to handle?
+ ArrowError::InvalidArgumentError(format!(
+ "Expected a ShreddedVariantFieldArray, got {:?} instead",
+ field.data_type(),
+ ))
+ })?;
+
+ Ok(ShreddedPathStep::Success(field.shredding_state()))
+ }
+ VariantPathElement::Index { .. } => {
+ // TODO: Support array indexing. Among other things, it will require slicing not
+ // only the array we have here, but also the corresponding metadata and null masks.
+ Err(ArrowError::NotYetImplemented(
+ "Pathing into shredded variant array index".into(),
+ ))
+ }
+ }
+}
+
+/// Follows the given path as far as possible through shredded variant fields. If the path ends on a
+/// shredded field, return it directly. Otherwise, use a row shredder to follow the rest of the path
+/// and extract the requested value on a per-row basis.
+fn shredded_get_path(
+ input: &VariantArray,
+ path: &[VariantPathElement<'_>],
+ as_type: Option<&DataType>,
+) -> Result<ArrayRef> {
+ // Helper that creates a new VariantArray from the given nested value and typed_value columns,
+ let make_target_variant = |value: Option<BinaryViewArray>, typed_value: Option<ArrayRef>| {
+ let metadata = input.metadata_field().clone();
+ let nulls = input.inner().nulls().cloned();
+ VariantArray::from_parts(
+ metadata,
+ value,
+ typed_value,
+ nulls,
+ )
+ };
+
+ // Helper that shreds a VariantArray to a specific type.
+ let shred_basic_variant = |target: VariantArray, path: VariantPath<'_>, as_type: Option<&DataType>| {
+ let mut builder = output::struct_output::make_shredding_row_builder(path, as_type)?;
+ for i in 0..target.len() {
+ if target.is_null(i) {
+ builder.append_null()?;
+ } else {
+ builder.append_value(&target.value(i))?;
+ }
+ }
+ builder.finish()
+ };
+
+ // Peel away the prefix of path elements that traverses the shredded parts of this variant
+ // column. Shredding will traverse the rest of the path on a per-row basis.
+ let mut shredding_state = input.shredding_state();
+ let mut path_index = 0;
+ for path_element in path {
+ match follow_shredded_path_element(shredding_state, path_element)? {
+ ShreddedPathStep::Success(state) => {
+ shredding_state = state;
+ path_index += 1;
+ continue;
+ }
+ ShreddedPathStep::Missing => {
+ let num_rows = input.len();
+ let arr = match as_type {
+ Some(data_type) => Arc::new(array::new_null_array(data_type, num_rows)) as _,
+ None => Arc::new(array::NullArray::new(num_rows)) as _,
+ };
+ return Ok(arr);
+ }
+ ShreddedPathStep::NotShredded => {
+ let target = make_target_variant(shredding_state.value_field().cloned(), None);
+ return shred_basic_variant(target, path[path_index..].into(), as_type);
+ }
+ };
+ }
+
+ // Path exhausted! Create a new `VariantArray` for the location we landed on.
+ let target = make_target_variant(
+ shredding_state.value_field().cloned(),
+ shredding_state.typed_value_field().cloned(),
+ );
+
+ // If our caller did not request any specific type, we can just return whatever we landed on.
+ let Some(data_type) = as_type else {
+ return Ok(Arc::new(target));
+ };
+
+ // Structs are special. Recurse into each field separately, hoping to follow the shredding even
+ // further, and build up the final struct from those individually shredded results.
+ if let DataType::Struct(fields) = data_type {
+ let children = fields
+ .iter()
+ .map(|field| {
+ shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ Some(field.data_type()),
+ )
+ })
+ .collect::<Result<Vec<_>>>()?;
+
+ return Ok(Arc::new(StructArray::try_new(
+ fields.clone(),
+ children,
+ target.nulls().cloned(),
+ )?));
+ }
+
+ // Not a struct, so directly shred the variant as the requested type
+ shred_basic_variant(target, VariantPath::default(), as_type)
+}
+
/// Returns an array with the specified path extracted from the variant values.
///
/// The return array type depends on the `as_type` field of the options parameter
/// 1. `as_type: None`: a VariantArray is returned. The values in this new VariantArray will point
/// to the specified path.
/// 2. `as_type: Some(<specific field>)`: an array of the specified type is returned.
+///
+/// TODO: How would a caller request a struct or list type where the fields/elements can be any
+/// variant? Caller can pass None as the requested type to fetch a specific path, but it would
+/// quickly become annoying (and inefficient) to call `variant_get` for each leaf value in a struct or
+/// list and then try to assemble the results.
pub fn variant_get(input: &ArrayRef, options: GetOptions) -> Result<ArrayRef> {
let variant_array: &VariantArray = input.as_any().downcast_ref().ok_or_else(|| {
ArrowError::InvalidArgumentError(
"expected a VariantArray as the input for variant_get".to_owned(),
)
})?;
- // Create the output writer based on the specified output options
- let output_builder = instantiate_output_builder(options.clone())?;
+ let GetOptions { as_type, path, .. } = &options;
- // Dispatch based on the shredding state of the input variant array
- match variant_array.shredding_state() {
- ShreddingState::PartiallyShredded {
- metadata,
- value,
- typed_value,
- } => output_builder.partially_shredded(variant_array, metadata, value, typed_value),
- ShreddingState::Typed {
- metadata,
- typed_value,
- } => output_builder.typed(variant_array, metadata, typed_value),
- ShreddingState::Unshredded { metadata, value } => {
- output_builder.unshredded(variant_array, metadata, value)
- }
- }
+ let as_type = as_type.as_ref().map(|f| f.data_type());
Review Comment:
This loses nullability of the passed-in `Field`? I'm not sure why that was needed in the first place or what impact it would have, tho?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -388,43 +525,720 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_object_field_access() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from("x"));
+ let result = variant_get(&array, options).unwrap();
+
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect x=1
+ assert_eq!(result_variant.value(0), Variant::Int32(1));
+ // Row 1: expect x=42
+ assert_eq!(result_variant.value(1), Variant::Int32(42));
+ }
+
+ /// Test extracting shredded object field with type conversion
+ #[test]
+ fn test_shredded_object_field_as_int32() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as Int32Array (type conversion)
+ let field = Field::new("x", DataType::Int32, false);
+ let options = GetOptions::new_with_path(VariantPath::from("x"))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get Int32Array
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
+ assert_eq!(&result, &expected);
+ }
- /// Builds struct arrays from component fields
+ /// Helper function to create a shredded variant array representing objects
+ ///
+ /// This creates an array that represents:
+ /// Row 0: {"x": 1, "y": "foo"} (x is shredded, y is in value field)
+ /// Row 1: {"x": 42} (x is shredded, perfect shredding)
Review Comment:
See comment above; other rows of interest would be:
* Int8 or Int16 value (which can be widened to Int32)
* Small Int64 value (which can be converted losslessly to Int32)
* Large Int64 value (which cannot be converted)
Even if we don't implement widening/narrowing casts, the test would at least document current behavior (and if we changed or expanded casting behavior later, the test would fail until updated to match)
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -388,43 +525,720 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_object_field_access() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from("x"));
+ let result = variant_get(&array, options).unwrap();
+
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect x=1
+ assert_eq!(result_variant.value(0), Variant::Int32(1));
+ // Row 1: expect x=42
+ assert_eq!(result_variant.value(1), Variant::Int32(42));
+ }
+
+ /// Test extracting shredded object field with type conversion
+ #[test]
+ fn test_shredded_object_field_as_int32() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as Int32Array (type conversion)
+ let field = Field::new("x", DataType::Int32, false);
+ let options = GetOptions::new_with_path(VariantPath::from("x"))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get Int32Array
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
+ assert_eq!(&result, &expected);
+ }
- /// Builds struct arrays from component fields
+ /// Helper function to create a shredded variant array representing objects
+ ///
+ /// This creates an array that represents:
+ /// Row 0: {"x": 1, "y": "foo"} (x is shredded, y is in value field)
+ /// Row 1: {"x": 42} (x is shredded, perfect shredding)
///
- /// TODO: move to arrow crate
- #[derive(Debug, Default, Clone)]
- struct StructArrayBuilder {
- fields: Vec<FieldRef>,
- arrays: Vec<ArrayRef>,
- nulls: Option<NullBuffer>,
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains object metadata
+ /// - typed_value: StructArray with field "x" (ShreddedVariantFieldArray)
+ /// - value: contains fallback for unshredded fields like {"y": "foo"}
+ /// - The "x" field has typed_value=Int32Array and value=NULL (perfect shredding)
+ fn shredded_object_with_x_field_variant_array() -> ArrayRef {
+ // Create the base metadata for objects
+ let (metadata, y_field_value) = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ // Build an object to get proper metadata
+ let mut obj = builder.new_object();
+ obj.insert("y", Variant::from("foo"));
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ // Create metadata array (same for both rows)
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2));
+
+ // Create the main value field per the 3-step shredding spec:
+ // Step 2: If field not in shredding schema, check value field
+ // Row 0: {"y": "foo"} (y is not shredded, stays in value for step 2)
+ // Row 1: {} (empty object - no unshredded fields)
+ let empty_object_value = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ let value_array = BinaryViewArray::from(vec![
+ Some(y_field_value.as_slice()), // Row 0 has {"y": "foo"}
+ Some(empty_object_value.as_slice()), // Row 1 has {}
+ ]);
+
+ // Create the "x" field as a ShreddedVariantFieldArray
+ // This represents the shredded Int32 values for the "x" field
+ let x_field_typed_value = Int32Array::from(vec![Some(1), Some(42)]);
+
+ // For perfect shredding of the x field, no "value" column, only typed_value
+ let x_field_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(x_field_typed_value))
+ .build();
+
+ // Wrap the x field struct in a ShreddedVariantFieldArray
+ let x_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(x_field_struct))
+ .expect("should create ShreddedVariantFieldArray");
+
+ // Create the main typed_value as a struct containing the "x" field
+ let typed_value_fields = Fields::from(vec![
+ Field::new("x", x_field_shredded.data_type().clone(), true)
+ ]);
+ let typed_value_struct = StructArray::try_new(
+ typed_value_fields,
+ vec![Arc::new(x_field_shredded)],
+ None, // No nulls - both rows have the object structure
+ ).unwrap();
+
+ // Create the main VariantArray
+ let main_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_struct))
+ .build();
+
+ Arc::new(
+ VariantArray::try_new(Arc::new(main_struct)).expect("should create variant array"),
+ )
}
- impl StructArrayBuilder {
- fn new() -> Self {
- Default::default()
+ /// Simple test to check if nested paths are supported by current implementation
+ #[test]
+ fn test_simple_nested_path_support() {
+ // Check: How does VariantPath parse different strings?
+ println!("Testing path parsing:");
+
+ let path_x = VariantPath::from("x");
+ let elements_x: Vec<_> = path_x.iter().collect();
+ println!(" 'x' -> {} elements: {:?}", elements_x.len(), elements_x);
+
+ let path_ax = VariantPath::from("a.x");
+ let elements_ax: Vec<_> = path_ax.iter().collect();
+ println!(" 'a.x' -> {} elements: {:?}", elements_ax.len(), elements_ax);
+
+ let path_ax_alt = VariantPath::from("$.a.x");
+ let elements_ax_alt: Vec<_> = path_ax_alt.iter().collect();
+ println!(" '$.a.x' -> {} elements: {:?}", elements_ax_alt.len(), elements_ax_alt);
+
+ let path_nested = VariantPath::from("a").join("x");
+ let elements_nested: Vec<_> = path_nested.iter().collect();
+ println!(" VariantPath::from('a').join('x') -> {} elements: {:?}", elements_nested.len(), elements_nested);
+
+ // Use your existing simple test data but try "a.x" instead of "x"
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test if variant_get with REAL nested path throws not implemented error
+ let real_nested_path = VariantPath::from("a").join("x");
+ let options = GetOptions::new_with_path(real_nested_path);
+ let result = variant_get(&array, options);
+
+ match result {
+ Ok(_) => {
+ println!("Nested path 'a.x' works unexpectedly!");
+ },
+ Err(e) => {
+ println!("Nested path 'a.x' error: {}", e);
+ if e.to_string().contains("not yet implemented")
+ || e.to_string().contains("NotYetImplemented") {
+ println!("This is expected - nested paths are not implemented");
+ return;
+ }
+ // Any other error is also expected for now
+ println!("This shows nested paths need implementation");
+ }
}
+ }
+
+ /// Test comprehensive variant_get scenarios with Int32 conversion
+ ///
+ /// Covers all combinations of:
+ /// - Input: Shredded vs Non-shredded VariantArrays
+ /// - Path depths: 0, 1, 2 levels
+ /// - Output: Always cast to Int32
+ #[test]
+ fn test_variant_get_comprehensive_int32() {
Review Comment:
Shouldn't this just be three unit tests? What do we gain by introducing this single-test wrapper for them?
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ }
+ } else {
+ // Non-empty paths: field access functionality
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a path builder that wraps a primitive builder
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ None => {
+ // Create a path builder that wraps a variant array builder
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ let path_builder = VariantPathRowBuilder {
Review Comment:
I guess this duplication is due to `VariantPathRowBuilder` being generic instead of taking a boxed dyn builder?
Is the code bloat worth it, as the number of supported types increases?
If we still want strong typing, we could perhaps pass the path down and use a helper to wrap the builder as needed?
```rust
fn boxed_with_path<'a>(
builder: impl VariantShreddingRowBuilder + 'a,
path: VariantPath<'a>,
) -> Box<dyn VariantShreddingRowBuilder + 'a> {
if path.is_empty() {
Box::new(builder)
} else {
Box::new(VariantPathRowBuilder { builder, path })
}
}
```
and then
```rust
match data_type {
None => {
let builder = VariantArrayShreddingRowBuilder::new(16);
boxed_with_path(builder, path)
}
Some(datatypes::DataType::Int32) => {
let builder = PrimitiveVariantShreddingRowBuilder { ... };
boxed_with_path(builder, path)
}
```
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ }
+ } else {
+ // Non-empty paths: field access functionality
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a path builder that wraps a primitive builder
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ None => {
+ // Create a path builder that wraps a variant array builder
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+#[allow(unused)]
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+#[allow(unused)]
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
+ builder: T,
+ path: VariantPath<'a>,
+}
+
+impl<T: VariantShreddingRowBuilder> VariantShreddingRowBuilder for VariantPathRowBuilder<'_, T> {
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null()
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(v) = value.get_path(&self.path) {
+ self.builder.append_value(&v)
+ } else {
+ self.builder.append_null()?;
+ Ok(false)
+ }
+ }
+ fn finish(&mut self) -> Result<ArrayRef> {
+ self.builder.finish()
+ }
+}
+
+/// Helper trait for converting `Variant` values to arrow primitive values.
+#[allow(unused)]
+trait VariantAsPrimitive<T: ArrowPrimitiveType> {
+ fn as_primitive(&self) -> Option<T::Native>;
+}
+impl VariantAsPrimitive<datatypes::Int32Type> for Variant<'_, '_> {
+ fn as_primitive(&self) -> Option<i32> {
+ self.as_int32()
+ }
+}
+impl VariantAsPrimitive<datatypes::Float64Type> for Variant<'_, '_> {
+ fn as_primitive(&self) -> Option<f64> {
+ self.as_f64()
+ }
+}
+
+/// Builder for shredding variant values to primitive values
+#[allow(unused)]
+struct PrimitiveVariantShreddingRowBuilder<T: ArrowPrimitiveType> {
+ builder: arrow::array::PrimitiveBuilder<T>,
+}
+
+impl<T> VariantShreddingRowBuilder for PrimitiveVariantShreddingRowBuilder<T>
+where
+ T: ArrowPrimitiveType,
+ for<'m, 'v> Variant<'m, 'v>: VariantAsPrimitive<T>,
+{
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null();
+ Ok(())
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(v) = value.as_primitive() {
+ self.builder.append_value(v);
+ Ok(true)
+ } else {
+ // append null on conversion failure (safe casting behavior)
+ // This matches the default CastOptions::safe = true behavior
+ // TODO: In future steps, respect CastOptions for safe vs unsafe casting
+ self.builder.append_null();
+ Ok(false)
+ }
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ Ok(Arc::new(self.builder.finish()))
+ }
+}
+
+/// Builder for creating VariantArray output (for path extraction without type conversion)
+#[allow(unused)]
+struct VariantArrayShreddingRowBuilder {
+ builder: VariantArrayBuilder,
+}
+
+impl VariantArrayShreddingRowBuilder {
+ fn new(capacity: usize) -> Self {
+ Self {
+ builder: VariantArrayBuilder::new(capacity),
+ }
+ }
+}
+
+impl VariantShreddingRowBuilder for VariantArrayShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null();
+ Ok(())
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ self.builder.append_variant(value.clone());
+ Ok(true)
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ // VariantArrayBuilder::build takes ownership, so we need to replace it
+ let builder = std::mem::replace(&mut self.builder, VariantArrayBuilder::new(0));
+ Ok(Arc::new(builder.build()))
+ }
+}
+
+/// Builder for appending raw binary variant values to a BinaryViewArray. It copies the bytes
+/// as-is, without any decoding.
+#[allow(unused)]
+struct BinaryVariantRowBuilder {
+ nulls: NullBufferBuilder,
+}
+
+impl VariantShreddingRowBuilder for BinaryVariantRowBuilder {
+ fn append_null(&mut self) -> Result<()> {
+ self.nulls.append_null();
+ Ok(())
+ }
+ fn append_value(&mut self, _value: &Variant<'_, '_>) -> Result<bool> {
+ // We need a way to convert a Variant directly to bytes. In particular, we want to just copy
+ // across the underlying value byte slice of a `Variant::Object` or `Variant::List`, without
+ // any interaction with a `VariantMetadata` (because we will just reuse the existing one).
+ //
+ // One could _probably_ emulate this with parquet_variant::VariantBuilder, but it would do a
+ // lot of unnecessary work and would also create a new metadata column we don't need.
+ todo!()
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ // What `finish` does will depend strongly on how `append_value` ends up working. But
+ // ultimately we'll create and return a `VariantArray` instance.
+ todo!()
+ }
+}
+
+/// Builder that extracts a struct. Casting failures produce NULL or error according to options.
+#[allow(unused)]
+struct StructVariantShreddingRowBuilder {
+ nulls: NullBufferBuilder,
+ field_builders: Vec<(FieldRef, Box<dyn VariantShreddingRowBuilder>)>,
+}
+
+impl VariantShreddingRowBuilder for StructVariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()> {
+ for (_, builder) in &mut self.field_builders {
+ builder.append_null()?;
+ }
+ self.nulls.append_null();
+ Ok(())
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ // Casting failure if it's not even an object.
+ let Variant::Object(value) = value else {
+ // TODO: handle casting failure
+ self.append_null()?;
+ return Ok(false);
+ };
+
+ // Process each field. If the field is missing, it becomes NULL. If the field is present,
+ // the child builder handles it from there, and a failed cast could produce NULL or error.
+ //
+ // TODO: This loop costs `O(m lg n)` where `m` is the number of fields in this builder and
+ // `n` is the number of fields in the variant object we're probing. Given that `m` and `n`
+ // could both be large -- indepentently of each other -- we should consider doing something
+ // more clever that bounds the cost to O(m + n).
+ for (field, builder) in &mut self.field_builders {
+ match value.get(field.name()) {
+ None => builder.append_null()?,
+ Some(v) => {
+ builder.append_value(&v)?;
+ }
+ }
+ }
+ self.nulls.append_non_null();
+ Ok(true)
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ let mut fields = Vec::with_capacity(self.field_builders.len());
+ let mut arrays = Vec::with_capacity(self.field_builders.len());
+ for (field, mut builder) in std::mem::take(&mut self.field_builders) {
+ fields.push(field);
+ arrays.push(builder.finish()?);
+ }
+ Ok(Arc::new(arrow::array::StructArray::try_new(
+ fields.into(),
+ arrays,
+ self.nulls.finish(),
+ )?))
+ }
+}
+
+/// Used for actual shredding of binary variant values into shredded variant values
+#[allow(unused)]
+struct ShreddedVariantRowBuilder {
Review Comment:
Should we just out leave this struct for now, and remove the code and types that support it?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -230,107 +390,138 @@ pub enum ShreddingState {
// https://github.com/apache/arrow-rs/issues/8088
// Missing { metadata: BinaryViewArray },
/// This variant has no typed_value field
- Unshredded {
- metadata: BinaryViewArray,
- value: BinaryViewArray,
- },
+ Unshredded { value: BinaryViewArray },
/// This variant has a typed_value field and no value field
/// meaning it is the shredded type
- Typed {
- metadata: BinaryViewArray,
- typed_value: ArrayRef,
- },
- /// Partially shredded:
- /// * value is an object
- /// * typed_value is a shredded object.
+ PerfectlyShredded { typed_value: ArrayRef },
+ /// Imperfectly shredded: Shredded values reside in `typed_value` while those that failed to
+ /// shred reside in `value`. Missing field values are NULL in both columns, while NULL primitive
+ /// values have NULL `typed_value` and `Variant::Null` in `value`.
///
- /// Note the spec says "Writers must not produce data where both value and
- /// typed_value are non-null, unless the Variant value is an object."
- PartiallyShredded {
- metadata: BinaryViewArray,
+ /// NOTE: A partially shredded struct is a special kind of imperfect shredding, where
+ /// `typed_value` and `value` are both non-NULL. The `typed_value` is a struct containing the
+ /// subset of fields for which shredding was attempted (each field will then have its own value
+ /// and/or typed_value sub-fields that indicate how shredding actually turned out). Meanwhile,
+ /// the `value` is a variant object containing the subset of fields for which shredding was
+ /// not even attempted.
+ ImperfectlyShredded {
value: BinaryViewArray,
typed_value: ArrayRef,
},
}
impl ShreddingState {
/// try to create a new `ShreddingState` from the given fields
- pub fn try_new(
- metadata: BinaryViewArray,
- value: Option<BinaryViewArray>,
- typed_value: Option<ArrayRef>,
- ) -> Result<Self, ArrowError> {
- match (metadata, value, typed_value) {
- (metadata, Some(value), Some(typed_value)) => Ok(Self::PartiallyShredded {
- metadata,
- value,
- typed_value,
- }),
- (metadata, Some(value), None) => Ok(Self::Unshredded { metadata, value }),
- (metadata, None, Some(typed_value)) => Ok(Self::Typed {
- metadata,
- typed_value,
- }),
- (_metadata_field, None, None) => Err(ArrowError::InvalidArgumentError(String::from(
+ pub fn try_new(inner: &StructArray) -> Result<Self, ArrowError> {
+ // Note the specification allows for any order so we must search by name
+
+ // Find the value field, if present
+ let value = inner
+ .column_by_name("value")
+ .map(|v| {
+ v.as_binary_view_opt().ok_or_else(|| {
+ ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ v.data_type()
+ ))
+ })
+ })
+ .transpose()?
+ .cloned();
+
+ // Find the typed_value field, if present
+ let typed_value = inner.column_by_name("typed_value").cloned();
+
+ match (value, typed_value) {
+ (Some(value), Some(typed_value)) => {
+ Ok(Self::ImperfectlyShredded { value, typed_value })
+ }
+ (Some(value), None) => Ok(Self::Unshredded { value }),
+ (None, Some(typed_value)) => Ok(Self::PerfectlyShredded { typed_value }),
+ (None, None) => Err(ArrowError::InvalidArgumentError(String::from(
"VariantArray has neither value nor typed_value field",
))),
}
}
- /// Return a reference to the metadata field
- pub fn metadata_field(&self) -> &BinaryViewArray {
- match self {
- ShreddingState::Unshredded { metadata, .. } => metadata,
- ShreddingState::Typed { metadata, .. } => metadata,
- ShreddingState::PartiallyShredded { metadata, .. } => metadata,
- }
- }
-
/// Return a reference to the value field, if present
pub fn value_field(&self) -> Option<&BinaryViewArray> {
match self {
ShreddingState::Unshredded { value, .. } => Some(value),
- ShreddingState::Typed { .. } => None,
- ShreddingState::PartiallyShredded { value, .. } => Some(value),
+ ShreddingState::PerfectlyShredded { .. } => None,
+ ShreddingState::ImperfectlyShredded { value, .. } => Some(value),
}
}
/// Return a reference to the typed_value field, if present
pub fn typed_value_field(&self) -> Option<&ArrayRef> {
match self {
ShreddingState::Unshredded { .. } => None,
- ShreddingState::Typed { typed_value, .. } => Some(typed_value),
- ShreddingState::PartiallyShredded { typed_value, .. } => Some(typed_value),
+ ShreddingState::PerfectlyShredded { typed_value, .. } => Some(typed_value),
+ ShreddingState::ImperfectlyShredded { typed_value, .. } => Some(typed_value),
}
}
/// Slice all the underlying arrays
pub fn slice(&self, offset: usize, length: usize) -> Self {
match self {
- ShreddingState::Unshredded { metadata, value } => ShreddingState::Unshredded {
- metadata: metadata.slice(offset, length),
- value: value.slice(offset, length),
- },
- ShreddingState::Typed {
- metadata,
- typed_value,
- } => ShreddingState::Typed {
- metadata: metadata.slice(offset, length),
- typed_value: typed_value.slice(offset, length),
- },
- ShreddingState::PartiallyShredded {
- metadata,
- value,
- typed_value,
- } => ShreddingState::PartiallyShredded {
- metadata: metadata.slice(offset, length),
+ ShreddingState::Unshredded { value } => ShreddingState::Unshredded {
value: value.slice(offset, length),
- typed_value: typed_value.slice(offset, length),
},
+ ShreddingState::PerfectlyShredded { typed_value } => {
+ ShreddingState::PerfectlyShredded {
+ typed_value: typed_value.slice(offset, length),
+ }
+ }
+ ShreddingState::ImperfectlyShredded { value, typed_value } => {
+ ShreddingState::ImperfectlyShredded {
+ value: value.slice(offset, length),
+ typed_value: typed_value.slice(offset, length),
+ }
+ }
}
}
}
+/// Builds struct arrays from component fields
+///
+/// TODO: move to arrow crate
+#[derive(Debug, Default, Clone)]
+pub struct StructArrayBuilder {
Review Comment:
Does it need to be `pub` right away? Or better to leave it `pub(crate)` until we're sure we want it to be part of the public API?
##########
parquet-variant/src/path.rs:
##########
@@ -95,10 +95,19 @@ impl<'a> From<Vec<VariantPathElement<'a>>> for VariantPath<'a> {
}
}
-/// Create from &str
+/// Create from &str with support for dot notation
impl<'a> From<&'a str> for VariantPath<'a> {
fn from(path: &'a str) -> Self {
- VariantPath::new(vec![path.into()])
+ // Support dot notation: "a.x" -> ["a", "x"]
+ if path.contains('.') {
Review Comment:
This is dangerous without some well-defined way to escape values. Otherwise, a column name that contains a dot is perfectly legal but would produce an incorrect path.
I believe Iceberg uses canonical jsonpath as the string path format, but I'm not sure if we want to take a stance in such low-level code?
delta-kernel-rs faced a similar dilemma and ended up defining macro magic that takes string literals in and parses them after verifying there are no special characters. By calling the macro, caller affirms it is a "simple" column path where splitting on dots is correct.
See this [derive macro](https://github.com/delta-io/delta-kernel-rs/blob/main/derive-macros/src/lib.rs#L9-L27) and the companion [proc macro](https://github.com/delta-io/delta-kernel-rs/blob/main/kernel/src/expressions/column_names.rs#L343-L376) that wraps it up nicely.
I don't know if we want to go to such lengths here tho. Probably better to just use the unambiguous long-form, or define a test helper for benefit of unit tests?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -204,6 +308,62 @@ impl VariantArray {
pub fn typed_value_field(&self) -> Option<&ArrayRef> {
self.shredding_state.typed_value_field()
}
+
+ /// Returns a reference to the underlying [`StructArray`].
+ pub fn inner(&self) -> &StructArray {
+ &self.inner
+ }
+}
+
+impl Array for ShreddedVariantFieldArray {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn to_data(&self) -> ArrayData {
+ self.inner.to_data()
+ }
+
+ fn into_data(self) -> ArrayData {
+ self.inner.into_data()
+ }
+
+ fn data_type(&self) -> &DataType {
+ self.inner.data_type()
+ }
+
+ fn slice(&self, offset: usize, length: usize) -> ArrayRef {
+ let inner = self.inner.slice(offset, length);
+ let shredding_state = self.shredding_state.slice(offset, length);
+ Arc::new(Self {
+ inner,
+ shredding_state,
+ })
+ }
+
+ fn len(&self) -> usize {
+ self.inner.len()
+ }
+
+ fn is_empty(&self) -> bool {
+ self.inner.is_empty()
+ }
+
+ fn offset(&self) -> usize {
+ self.inner.offset()
+ }
+
+ fn nulls(&self) -> Option<&NullBuffer> {
+ self.inner.nulls()
+ }
Review Comment:
We may need to override the [logical_nulls](https://docs.rs/arrow/latest/arrow/array/trait.Array.html#method.logical_nulls) method.
According to the [shredding spec](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#objects), the parquet group we use `ShreddedVariantFieldArray` to represent is `required` -- physically non-nullable (`nulls` should return `None`) -- and SQL NULL is inferred by `value` and `typed_value` both being physically NULL:
```
optional group event (VARIANT) { # [a VariantArray]
required binary metadata;
optional binary value; # a variant, expected to be an object
optional group typed_value { # shredded fields for the variant object
required group event_type { # shredded field for event_type [a ShreddedVariantFieldArray]
optional binary value;
optional binary typed_value (STRING);
}
required group event_ts { # shredded field for event_ts [a ShreddedVariantFieldArray]
optional binary value;
optional int64 typed_value (TIMESTAMP(true, MICROS));
}
}
}
```
and
<img width="671" height="89" alt="image" src="https://github.com/user-attachments/assets/cc40a6b3-eedf-480f-a987-7599779f49d9" />
Tho I suppose `nulls` could possibly still be present if e.g. `event.typed_value` is NULL for some rows? I'm a bit fuzzy on that part.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -230,107 +390,138 @@ pub enum ShreddingState {
// https://github.com/apache/arrow-rs/issues/8088
// Missing { metadata: BinaryViewArray },
/// This variant has no typed_value field
- Unshredded {
- metadata: BinaryViewArray,
- value: BinaryViewArray,
- },
+ Unshredded { value: BinaryViewArray },
/// This variant has a typed_value field and no value field
/// meaning it is the shredded type
- Typed {
- metadata: BinaryViewArray,
- typed_value: ArrayRef,
- },
- /// Partially shredded:
- /// * value is an object
- /// * typed_value is a shredded object.
+ PerfectlyShredded { typed_value: ArrayRef },
+ /// Imperfectly shredded: Shredded values reside in `typed_value` while those that failed to
+ /// shred reside in `value`. Missing field values are NULL in both columns, while NULL primitive
+ /// values have NULL `typed_value` and `Variant::Null` in `value`.
///
- /// Note the spec says "Writers must not produce data where both value and
- /// typed_value are non-null, unless the Variant value is an object."
- PartiallyShredded {
- metadata: BinaryViewArray,
+ /// NOTE: A partially shredded struct is a special kind of imperfect shredding, where
+ /// `typed_value` and `value` are both non-NULL. The `typed_value` is a struct containing the
+ /// subset of fields for which shredding was attempted (each field will then have its own value
+ /// and/or typed_value sub-fields that indicate how shredding actually turned out). Meanwhile,
+ /// the `value` is a variant object containing the subset of fields for which shredding was
+ /// not even attempted.
+ ImperfectlyShredded {
value: BinaryViewArray,
typed_value: ArrayRef,
},
}
impl ShreddingState {
/// try to create a new `ShreddingState` from the given fields
- pub fn try_new(
- metadata: BinaryViewArray,
- value: Option<BinaryViewArray>,
- typed_value: Option<ArrayRef>,
- ) -> Result<Self, ArrowError> {
- match (metadata, value, typed_value) {
- (metadata, Some(value), Some(typed_value)) => Ok(Self::PartiallyShredded {
- metadata,
- value,
- typed_value,
- }),
- (metadata, Some(value), None) => Ok(Self::Unshredded { metadata, value }),
- (metadata, None, Some(typed_value)) => Ok(Self::Typed {
- metadata,
- typed_value,
- }),
- (_metadata_field, None, None) => Err(ArrowError::InvalidArgumentError(String::from(
+ pub fn try_new(inner: &StructArray) -> Result<Self, ArrowError> {
+ // Note the specification allows for any order so we must search by name
+
+ // Find the value field, if present
+ let value = inner
+ .column_by_name("value")
+ .map(|v| {
+ v.as_binary_view_opt().ok_or_else(|| {
+ ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ v.data_type()
+ ))
+ })
+ })
+ .transpose()?
+ .cloned();
+
+ // Find the typed_value field, if present
+ let typed_value = inner.column_by_name("typed_value").cloned();
+
+ match (value, typed_value) {
+ (Some(value), Some(typed_value)) => {
+ Ok(Self::ImperfectlyShredded { value, typed_value })
+ }
+ (Some(value), None) => Ok(Self::Unshredded { value }),
+ (None, Some(typed_value)) => Ok(Self::PerfectlyShredded { typed_value }),
+ (None, None) => Err(ArrowError::InvalidArgumentError(String::from(
Review Comment:
I'm still on the fence whether an enum with four variants is actually helpful, vs.
```rust
pub struct ShreddingState {
value: Option<BinaryViewArray>,
typed_value: Option<ArrayRef>,
}
```
In my prototyping, `ShreddingState::value_field()` and `ShreddingState::typed_value_field()` methods were far easier to use in most settings, instead of matching on specific enum variants. Even internally, most methods of `ShreddingState` (like this code here) don't really benefit from the enum variants?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -230,107 +390,138 @@ pub enum ShreddingState {
// https://github.com/apache/arrow-rs/issues/8088
// Missing { metadata: BinaryViewArray },
/// This variant has no typed_value field
- Unshredded {
- metadata: BinaryViewArray,
- value: BinaryViewArray,
- },
+ Unshredded { value: BinaryViewArray },
/// This variant has a typed_value field and no value field
/// meaning it is the shredded type
- Typed {
- metadata: BinaryViewArray,
- typed_value: ArrayRef,
- },
- /// Partially shredded:
- /// * value is an object
- /// * typed_value is a shredded object.
+ PerfectlyShredded { typed_value: ArrayRef },
+ /// Imperfectly shredded: Shredded values reside in `typed_value` while those that failed to
+ /// shred reside in `value`. Missing field values are NULL in both columns, while NULL primitive
+ /// values have NULL `typed_value` and `Variant::Null` in `value`.
///
- /// Note the spec says "Writers must not produce data where both value and
- /// typed_value are non-null, unless the Variant value is an object."
- PartiallyShredded {
- metadata: BinaryViewArray,
+ /// NOTE: A partially shredded struct is a special kind of imperfect shredding, where
+ /// `typed_value` and `value` are both non-NULL. The `typed_value` is a struct containing the
+ /// subset of fields for which shredding was attempted (each field will then have its own value
+ /// and/or typed_value sub-fields that indicate how shredding actually turned out). Meanwhile,
+ /// the `value` is a variant object containing the subset of fields for which shredding was
+ /// not even attempted.
+ ImperfectlyShredded {
value: BinaryViewArray,
typed_value: ArrayRef,
},
}
impl ShreddingState {
/// try to create a new `ShreddingState` from the given fields
- pub fn try_new(
- metadata: BinaryViewArray,
- value: Option<BinaryViewArray>,
- typed_value: Option<ArrayRef>,
- ) -> Result<Self, ArrowError> {
- match (metadata, value, typed_value) {
- (metadata, Some(value), Some(typed_value)) => Ok(Self::PartiallyShredded {
- metadata,
- value,
- typed_value,
- }),
- (metadata, Some(value), None) => Ok(Self::Unshredded { metadata, value }),
- (metadata, None, Some(typed_value)) => Ok(Self::Typed {
- metadata,
- typed_value,
- }),
- (_metadata_field, None, None) => Err(ArrowError::InvalidArgumentError(String::from(
+ pub fn try_new(inner: &StructArray) -> Result<Self, ArrowError> {
+ // Note the specification allows for any order so we must search by name
+
+ // Find the value field, if present
+ let value = inner
+ .column_by_name("value")
+ .map(|v| {
+ v.as_binary_view_opt().ok_or_else(|| {
+ ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ v.data_type()
+ ))
+ })
+ })
+ .transpose()?
+ .cloned();
+
+ // Find the typed_value field, if present
+ let typed_value = inner.column_by_name("typed_value").cloned();
+
+ match (value, typed_value) {
+ (Some(value), Some(typed_value)) => {
+ Ok(Self::ImperfectlyShredded { value, typed_value })
+ }
+ (Some(value), None) => Ok(Self::Unshredded { value }),
+ (None, Some(typed_value)) => Ok(Self::PerfectlyShredded { typed_value }),
+ (None, None) => Err(ArrowError::InvalidArgumentError(String::from(
"VariantArray has neither value nor typed_value field",
))),
}
}
- /// Return a reference to the metadata field
- pub fn metadata_field(&self) -> &BinaryViewArray {
- match self {
- ShreddingState::Unshredded { metadata, .. } => metadata,
- ShreddingState::Typed { metadata, .. } => metadata,
- ShreddingState::PartiallyShredded { metadata, .. } => metadata,
- }
- }
-
/// Return a reference to the value field, if present
pub fn value_field(&self) -> Option<&BinaryViewArray> {
match self {
ShreddingState::Unshredded { value, .. } => Some(value),
- ShreddingState::Typed { .. } => None,
- ShreddingState::PartiallyShredded { value, .. } => Some(value),
+ ShreddingState::PerfectlyShredded { .. } => None,
+ ShreddingState::ImperfectlyShredded { value, .. } => Some(value),
Review Comment:
```rust
ShreddingState::Unshredded { value }
| ShreddingState::ImperfectlyShredded { value, .. } => Some(value),
ShreddingState::PerfectlyShredded { .. } => None,
```
(and eventually the new `ShreddingState::AllNull` enum variant joins the second match arm)
(similar story below for `typed_value`)
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ if shredding_state.value_field().is_none() {
+ ShreddedPathStep::Missing
+ } else {
+ ShreddedPathStep::NotShredded
+ }
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ let Some(field) = typed_value
+ .as_any()
+ .downcast_ref::<StructArray>()
Review Comment:
If we start honoring strict cast options, should failure to downcast produce a (logical cast failure) error instead of NULL?
(meanwhile, I'm pretty sure missing column in a present struct is just missing, not wrong, and it's correct to return Ok for that part)
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ if shredding_state.value_field().is_none() {
+ ShreddedPathStep::Missing
+ } else {
+ ShreddedPathStep::NotShredded
+ }
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ let Some(field) = typed_value
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .and_then(|typed_value| typed_value.column_by_name(name))
+ else {
+ return Ok(missing_path_step());
+ };
+
+ let field = field
+ .as_any()
+ .downcast_ref::<ShreddedVariantFieldArray>()
+ .ok_or_else(|| {
+ // TODO: Should we blow up? Or just end the traversal and let the normal
+ // variant pathing code sort out the mess that it must anyway be
+ // prepared to handle?
Review Comment:
@alamb Very interested to hear your thoughts on this?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
Review Comment:
Should we file a follow-up issue to track this TODO?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
Review Comment:
```suggestion
// If the requested path element is not present in `typed_value`, and `value` is missing, then
```
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
Review Comment:
This module should probably be renamed as `row_builder.rs`?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ if shredding_state.value_field().is_none() {
+ ShreddedPathStep::Missing
+ } else {
+ ShreddedPathStep::NotShredded
+ }
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ let Some(field) = typed_value
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .and_then(|typed_value| typed_value.column_by_name(name))
+ else {
+ return Ok(missing_path_step());
+ };
+
+ let field = field
+ .as_any()
+ .downcast_ref::<ShreddedVariantFieldArray>()
+ .ok_or_else(|| {
+ // TODO: Should we blow up? Or just end the traversal and let the normal
+ // variant pathing code sort out the mess that it must anyway be
+ // prepared to handle?
+ ArrowError::InvalidArgumentError(format!(
+ "Expected a ShreddedVariantFieldArray, got {:?} instead",
+ field.data_type(),
+ ))
+ })?;
+
+ Ok(ShreddedPathStep::Success(field.shredding_state()))
+ }
+ VariantPathElement::Index { .. } => {
+ // TODO: Support array indexing. Among other things, it will require slicing not
+ // only the array we have here, but also the corresponding metadata and null masks.
+ Err(ArrowError::NotYetImplemented(
+ "Pathing into shredded variant array index".into(),
+ ))
+ }
+ }
+}
+
+/// Follows the given path as far as possible through shredded variant fields. If the path ends on a
+/// shredded field, return it directly. Otherwise, use a row shredder to follow the rest of the path
+/// and extract the requested value on a per-row basis.
+fn shredded_get_path(
+ input: &VariantArray,
+ path: &[VariantPathElement<'_>],
+ as_type: Option<&DataType>,
+) -> Result<ArrayRef> {
+ // Helper that creates a new VariantArray from the given nested value and typed_value columns,
+ let make_target_variant = |value: Option<BinaryViewArray>, typed_value: Option<ArrayRef>| {
+ let metadata = input.metadata_field().clone();
+ let nulls = input.inner().nulls().cloned();
+ VariantArray::from_parts(
+ metadata,
+ value,
+ typed_value,
+ nulls,
+ )
+ };
+
+ // Helper that shreds a VariantArray to a specific type.
+ let shred_basic_variant = |target: VariantArray, path: VariantPath<'_>, as_type: Option<&DataType>| {
+ let mut builder = output::struct_output::make_shredding_row_builder(path, as_type)?;
+ for i in 0..target.len() {
+ if target.is_null(i) {
+ builder.append_null()?;
+ } else {
+ builder.append_value(&target.value(i))?;
+ }
+ }
+ builder.finish()
+ };
+
+ // Peel away the prefix of path elements that traverses the shredded parts of this variant
+ // column. Shredding will traverse the rest of the path on a per-row basis.
+ let mut shredding_state = input.shredding_state();
+ let mut path_index = 0;
+ for path_element in path {
+ match follow_shredded_path_element(shredding_state, path_element)? {
+ ShreddedPathStep::Success(state) => {
+ shredding_state = state;
+ path_index += 1;
+ continue;
+ }
+ ShreddedPathStep::Missing => {
+ let num_rows = input.len();
+ let arr = match as_type {
+ Some(data_type) => Arc::new(array::new_null_array(data_type, num_rows)) as _,
+ None => Arc::new(array::NullArray::new(num_rows)) as _,
+ };
+ return Ok(arr);
+ }
+ ShreddedPathStep::NotShredded => {
+ let target = make_target_variant(shredding_state.value_field().cloned(), None);
+ return shred_basic_variant(target, path[path_index..].into(), as_type);
+ }
+ };
+ }
+
+ // Path exhausted! Create a new `VariantArray` for the location we landed on.
+ let target = make_target_variant(
+ shredding_state.value_field().cloned(),
+ shredding_state.typed_value_field().cloned(),
+ );
+
+ // If our caller did not request any specific type, we can just return whatever we landed on.
+ let Some(data_type) = as_type else {
+ return Ok(Arc::new(target));
+ };
+
+ // Structs are special. Recurse into each field separately, hoping to follow the shredding even
+ // further, and build up the final struct from those individually shredded results.
+ if let DataType::Struct(fields) = data_type {
+ let children = fields
+ .iter()
+ .map(|field| {
+ shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ Some(field.data_type()),
+ )
+ })
+ .collect::<Result<Vec<_>>>()?;
+
+ return Ok(Arc::new(StructArray::try_new(
+ fields.clone(),
+ children,
+ target.nulls().cloned(),
Review Comment:
As above, I'm not certain this is the complete null mask?
(null masks make my head hurt... not sure what the correct approach should be here)
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -388,43 +525,720 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_object_field_access() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from("x"));
+ let result = variant_get(&array, options).unwrap();
+
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect x=1
+ assert_eq!(result_variant.value(0), Variant::Int32(1));
+ // Row 1: expect x=42
+ assert_eq!(result_variant.value(1), Variant::Int32(42));
+ }
+
+ /// Test extracting shredded object field with type conversion
+ #[test]
+ fn test_shredded_object_field_as_int32() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as Int32Array (type conversion)
+ let field = Field::new("x", DataType::Int32, false);
+ let options = GetOptions::new_with_path(VariantPath::from("x"))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get Int32Array
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
+ assert_eq!(&result, &expected);
+ }
- /// Builds struct arrays from component fields
+ /// Helper function to create a shredded variant array representing objects
+ ///
+ /// This creates an array that represents:
+ /// Row 0: {"x": 1, "y": "foo"} (x is shredded, y is in value field)
+ /// Row 1: {"x": 42} (x is shredded, perfect shredding)
///
- /// TODO: move to arrow crate
- #[derive(Debug, Default, Clone)]
- struct StructArrayBuilder {
- fields: Vec<FieldRef>,
- arrays: Vec<ArrayRef>,
- nulls: Option<NullBuffer>,
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains object metadata
+ /// - typed_value: StructArray with field "x" (ShreddedVariantFieldArray)
+ /// - value: contains fallback for unshredded fields like {"y": "foo"}
+ /// - The "x" field has typed_value=Int32Array and value=NULL (perfect shredding)
+ fn shredded_object_with_x_field_variant_array() -> ArrayRef {
+ // Create the base metadata for objects
+ let (metadata, y_field_value) = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ // Build an object to get proper metadata
+ let mut obj = builder.new_object();
+ obj.insert("y", Variant::from("foo"));
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ // Create metadata array (same for both rows)
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2));
Review Comment:
The spec [requires](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#variant-metadata) the metadata to contain `"x"`:
> All `value` columns within the Variant must use the same `metadata`. All field names of a Variant, whether shredded or not, must be present in the metadata.
(needed for efficient unshredding)
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ if shredding_state.value_field().is_none() {
+ ShreddedPathStep::Missing
+ } else {
+ ShreddedPathStep::NotShredded
+ }
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ let Some(field) = typed_value
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .and_then(|typed_value| typed_value.column_by_name(name))
+ else {
+ return Ok(missing_path_step());
+ };
+
+ let field = field
+ .as_any()
+ .downcast_ref::<ShreddedVariantFieldArray>()
+ .ok_or_else(|| {
+ // TODO: Should we blow up? Or just end the traversal and let the normal
+ // variant pathing code sort out the mess that it must anyway be
+ // prepared to handle?
+ ArrowError::InvalidArgumentError(format!(
+ "Expected a ShreddedVariantFieldArray, got {:?} instead",
+ field.data_type(),
+ ))
+ })?;
+
+ Ok(ShreddedPathStep::Success(field.shredding_state()))
+ }
+ VariantPathElement::Index { .. } => {
+ // TODO: Support array indexing. Among other things, it will require slicing not
+ // only the array we have here, but also the corresponding metadata and null masks.
+ Err(ArrowError::NotYetImplemented(
+ "Pathing into shredded variant array index".into(),
+ ))
+ }
+ }
+}
+
+/// Follows the given path as far as possible through shredded variant fields. If the path ends on a
+/// shredded field, return it directly. Otherwise, use a row shredder to follow the rest of the path
+/// and extract the requested value on a per-row basis.
+fn shredded_get_path(
+ input: &VariantArray,
+ path: &[VariantPathElement<'_>],
+ as_type: Option<&DataType>,
+) -> Result<ArrayRef> {
+ // Helper that creates a new VariantArray from the given nested value and typed_value columns,
+ let make_target_variant = |value: Option<BinaryViewArray>, typed_value: Option<ArrayRef>| {
+ let metadata = input.metadata_field().clone();
+ let nulls = input.inner().nulls().cloned();
+ VariantArray::from_parts(
+ metadata,
+ value,
+ typed_value,
+ nulls,
+ )
+ };
+
+ // Helper that shreds a VariantArray to a specific type.
+ let shred_basic_variant = |target: VariantArray, path: VariantPath<'_>, as_type: Option<&DataType>| {
+ let mut builder = output::struct_output::make_shredding_row_builder(path, as_type)?;
+ for i in 0..target.len() {
+ if target.is_null(i) {
+ builder.append_null()?;
+ } else {
+ builder.append_value(&target.value(i))?;
+ }
+ }
+ builder.finish()
+ };
+
+ // Peel away the prefix of path elements that traverses the shredded parts of this variant
+ // column. Shredding will traverse the rest of the path on a per-row basis.
+ let mut shredding_state = input.shredding_state();
+ let mut path_index = 0;
+ for path_element in path {
+ match follow_shredded_path_element(shredding_state, path_element)? {
+ ShreddedPathStep::Success(state) => {
+ shredding_state = state;
+ path_index += 1;
+ continue;
+ }
+ ShreddedPathStep::Missing => {
+ let num_rows = input.len();
+ let arr = match as_type {
+ Some(data_type) => Arc::new(array::new_null_array(data_type, num_rows)) as _,
+ None => Arc::new(array::NullArray::new(num_rows)) as _,
+ };
+ return Ok(arr);
+ }
+ ShreddedPathStep::NotShredded => {
+ let target = make_target_variant(shredding_state.value_field().cloned(), None);
+ return shred_basic_variant(target, path[path_index..].into(), as_type);
+ }
+ };
+ }
+
+ // Path exhausted! Create a new `VariantArray` for the location we landed on.
+ let target = make_target_variant(
+ shredding_state.value_field().cloned(),
+ shredding_state.typed_value_field().cloned(),
+ );
+
+ // If our caller did not request any specific type, we can just return whatever we landed on.
+ let Some(data_type) = as_type else {
+ return Ok(Arc::new(target));
+ };
+
+ // Structs are special. Recurse into each field separately, hoping to follow the shredding even
+ // further, and build up the final struct from those individually shredded results.
+ if let DataType::Struct(fields) = data_type {
+ let children = fields
+ .iter()
+ .map(|field| {
+ shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ Some(field.data_type()),
+ )
+ })
+ .collect::<Result<Vec<_>>>()?;
+
+ return Ok(Arc::new(StructArray::try_new(
+ fields.clone(),
+ children,
+ target.nulls().cloned(),
+ )?));
+ }
+
+ // Not a struct, so directly shred the variant as the requested type
+ shred_basic_variant(target, VariantPath::default(), as_type)
+}
+
/// Returns an array with the specified path extracted from the variant values.
///
/// The return array type depends on the `as_type` field of the options parameter
/// 1. `as_type: None`: a VariantArray is returned. The values in this new VariantArray will point
/// to the specified path.
/// 2. `as_type: Some(<specific field>)`: an array of the specified type is returned.
+///
+/// TODO: How would a caller request a struct or list type where the fields/elements can be any
+/// variant? Caller can pass None as the requested type to fetch a specific path, but it would
+/// quickly become annoying (and inefficient) to call `variant_get` for each leaf value in a struct or
+/// list and then try to assemble the results.
pub fn variant_get(input: &ArrayRef, options: GetOptions) -> Result<ArrayRef> {
let variant_array: &VariantArray = input.as_any().downcast_ref().ok_or_else(|| {
ArrowError::InvalidArgumentError(
"expected a VariantArray as the input for variant_get".to_owned(),
)
})?;
- // Create the output writer based on the specified output options
- let output_builder = instantiate_output_builder(options.clone())?;
+ let GetOptions { as_type, path, .. } = &options;
Review Comment:
TODO: handle the actual cast options.
Especially strict casting that should return error instead of NULL on cast failure?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,50 +15,208 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element 's not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ if shredding_state.value_field().is_none() {
+ ShreddedPathStep::Missing
+ } else {
+ ShreddedPathStep::NotShredded
+ }
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ let Some(field) = typed_value
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .and_then(|typed_value| typed_value.column_by_name(name))
+ else {
+ return Ok(missing_path_step());
+ };
+
+ let field = field
+ .as_any()
+ .downcast_ref::<ShreddedVariantFieldArray>()
+ .ok_or_else(|| {
+ // TODO: Should we blow up? Or just end the traversal and let the normal
+ // variant pathing code sort out the mess that it must anyway be
+ // prepared to handle?
+ ArrowError::InvalidArgumentError(format!(
+ "Expected a ShreddedVariantFieldArray, got {:?} instead",
+ field.data_type(),
+ ))
+ })?;
+
+ Ok(ShreddedPathStep::Success(field.shredding_state()))
+ }
+ VariantPathElement::Index { .. } => {
+ // TODO: Support array indexing. Among other things, it will require slicing not
+ // only the array we have here, but also the corresponding metadata and null masks.
+ Err(ArrowError::NotYetImplemented(
+ "Pathing into shredded variant array index".into(),
+ ))
+ }
+ }
+}
+
+/// Follows the given path as far as possible through shredded variant fields. If the path ends on a
+/// shredded field, return it directly. Otherwise, use a row shredder to follow the rest of the path
+/// and extract the requested value on a per-row basis.
+fn shredded_get_path(
+ input: &VariantArray,
+ path: &[VariantPathElement<'_>],
+ as_type: Option<&DataType>,
+) -> Result<ArrayRef> {
+ // Helper that creates a new VariantArray from the given nested value and typed_value columns,
+ let make_target_variant = |value: Option<BinaryViewArray>, typed_value: Option<ArrayRef>| {
+ let metadata = input.metadata_field().clone();
+ let nulls = input.inner().nulls().cloned();
+ VariantArray::from_parts(
+ metadata,
+ value,
+ typed_value,
+ nulls,
+ )
+ };
+
+ // Helper that shreds a VariantArray to a specific type.
+ let shred_basic_variant = |target: VariantArray, path: VariantPath<'_>, as_type: Option<&DataType>| {
+ let mut builder = output::struct_output::make_shredding_row_builder(path, as_type)?;
+ for i in 0..target.len() {
+ if target.is_null(i) {
+ builder.append_null()?;
+ } else {
+ builder.append_value(&target.value(i))?;
+ }
+ }
+ builder.finish()
+ };
+
+ // Peel away the prefix of path elements that traverses the shredded parts of this variant
+ // column. Shredding will traverse the rest of the path on a per-row basis.
+ let mut shredding_state = input.shredding_state();
+ let mut path_index = 0;
+ for path_element in path {
+ match follow_shredded_path_element(shredding_state, path_element)? {
+ ShreddedPathStep::Success(state) => {
+ shredding_state = state;
+ path_index += 1;
+ continue;
+ }
+ ShreddedPathStep::Missing => {
+ let num_rows = input.len();
+ let arr = match as_type {
+ Some(data_type) => Arc::new(array::new_null_array(data_type, num_rows)) as _,
+ None => Arc::new(array::NullArray::new(num_rows)) as _,
+ };
+ return Ok(arr);
+ }
+ ShreddedPathStep::NotShredded => {
+ let target = make_target_variant(shredding_state.value_field().cloned(), None);
+ return shred_basic_variant(target, path[path_index..].into(), as_type);
+ }
+ };
+ }
+
+ // Path exhausted! Create a new `VariantArray` for the location we landed on.
+ let target = make_target_variant(
+ shredding_state.value_field().cloned(),
+ shredding_state.typed_value_field().cloned(),
+ );
+
+ // If our caller did not request any specific type, we can just return whatever we landed on.
+ let Some(data_type) = as_type else {
+ return Ok(Arc::new(target));
+ };
+
+ // Structs are special. Recurse into each field separately, hoping to follow the shredding even
+ // further, and build up the final struct from those individually shredded results.
+ if let DataType::Struct(fields) = data_type {
+ let children = fields
+ .iter()
+ .map(|field| {
+ shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ Some(field.data_type()),
+ )
+ })
+ .collect::<Result<Vec<_>>>()?;
+
+ return Ok(Arc::new(StructArray::try_new(
+ fields.clone(),
+ children,
+ target.nulls().cloned(),
+ )?));
+ }
+
+ // Not a struct, so directly shred the variant as the requested type
+ shred_basic_variant(target, VariantPath::default(), as_type)
+}
+
/// Returns an array with the specified path extracted from the variant values.
///
/// The return array type depends on the `as_type` field of the options parameter
/// 1. `as_type: None`: a VariantArray is returned. The values in this new VariantArray will point
/// to the specified path.
/// 2. `as_type: Some(<specific field>)`: an array of the specified type is returned.
+///
+/// TODO: How would a caller request a struct or list type where the fields/elements can be any
+/// variant? Caller can pass None as the requested type to fetch a specific path, but it would
+/// quickly become annoying (and inefficient) to call `variant_get` for each leaf value in a struct or
+/// list and then try to assemble the results.
Review Comment:
I think we figured out that we need a second method that takes a data type as input, rewrites each binary variant column to match the actual shredding schema for that field, and returns the result. Which can then be passed to `variant_get` for efficient processing.
@alamb is there already a tracking issue for this?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -388,43 +525,720 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
Review Comment:
we should probably have two more rows, where `x` is wrong-typed as `Variant::Null` and e.g. `Variant::String`? And a fifth row, where `x` is missing (SQL NULL)?
(it matters more for the second test below, but this test can at least confirm the types match expectations)
##########
parquet-variant-compute/src/variant_get/output/variant.rs:
##########
@@ -70,8 +71,81 @@ impl OutputBuilder for VariantOutputBuilder<'_> {
array_builder.append_variant(Variant::from(int_value));
}
}
+ DataType::Struct(_) => {
Review Comment:
The `VariantOutputBuilder` is marked `#[allow(unused)]` in this PR?
Does anything actually exercise this new code?
(AFAICT, `variant_get` was the only caller of `instantiate_output_builder`, and this PR removes that call site)
##########
parquet-variant-compute/src/variant_get/output/variant.rs:
##########
@@ -70,8 +71,81 @@ impl OutputBuilder for VariantOutputBuilder<'_> {
array_builder.append_variant(Variant::from(int_value));
}
}
+ DataType::Struct(_) => {
+ // For shredded objects with struct typed_value, we need sophisticated field access
+ // according to the shredding specification. This handles cases like:
+ // 1. Field exists in shredded structure -> extract from typed_value
+ // 2. Field missing from shredded structure -> fallback to value field or return null
+ // 3. Value field has nulls (perfect shredding) -> return null for missing fields
+
+ use parquet_variant::VariantPath;
+
+ // Extract the field name from the path
+ let path_elements: Vec<_> = self.path.iter().collect();
+ if path_elements.is_empty() {
+ // No specific field requested, this shouldn't happen in our context
+ return Err(ArrowError::InvalidArgumentError(
+ "DataType::Struct case with empty path not supported".to_string()
+ ));
+ }
+
+ // Get the first path element (field name)
+ if let parquet_variant::VariantPathElement::Field { name: field_name } = &path_elements[0] {
+ // Check if the field exists in the shredded structure
+ let struct_array = typed_value.as_any().downcast_ref::<arrow::array::StructArray>()
+ .ok_or_else(|| ArrowError::InvalidArgumentError(
+ "Expected StructArray for DataType::Struct typed_value".to_string()
+ ))?;
+
+ if let Some(_field_column) = struct_array.column_by_name(field_name) {
+ // Field exists in shredded structure - this should be handled by the normal
+ // shredded path traversal, not this fallback case. If we're here, there's
+ // likely an issue with the shredded path logic.
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Field '{}' exists in shredded structure but ended up in struct fallback case",
+ field_name
+ )));
+ } else {
+ // Field does NOT exist in shredded structure
+ // According to shredding spec: check value field for fallback
+
+ let value_field = variant_array.value_field();
+ if value_field.is_none() {
+ // No value field means perfect shredding - missing field = null
+ let num_rows = variant_array.len();
+ return Ok(Arc::new(array::NullArray::new(num_rows)) as ArrayRef);
+ }
+
+ // Value field exists - need to extract field from unshredded objects
+ let mut array_builder = VariantArrayBuilder::new(variant_array.len());
+
+ for i in 0..variant_array.len() {
+ if variant_array.is_null(i) {
+ array_builder.append_null();
+ continue;
+ }
+
+ // Get the variant for this row
+ let row_variant = variant_array.value(i);
+
+ // Extract the field from the variant object
+ if let Some(field_variant) = row_variant.get_path(&VariantPath::from(field_name.as_ref())) {
+ array_builder.append_variant(field_variant);
+ } else {
+ // Field doesn't exist in this object -> null
+ array_builder.append_null();
+ }
+ }
+
+ return Ok(Arc::new(array_builder.build()));
+ }
+ } else {
Review Comment:
Again, `else` not needed here? (just makes the code harder to follow IMO).
##########
parquet-variant-compute/src/variant_get/output/variant.rs:
##########
@@ -70,8 +71,81 @@ impl OutputBuilder for VariantOutputBuilder<'_> {
array_builder.append_variant(Variant::from(int_value));
}
}
+ DataType::Struct(_) => {
+ // For shredded objects with struct typed_value, we need sophisticated field access
+ // according to the shredding specification. This handles cases like:
+ // 1. Field exists in shredded structure -> extract from typed_value
+ // 2. Field missing from shredded structure -> fallback to value field or return null
+ // 3. Value field has nulls (perfect shredding) -> return null for missing fields
+
+ use parquet_variant::VariantPath;
+
+ // Extract the field name from the path
+ let path_elements: Vec<_> = self.path.iter().collect();
+ if path_elements.is_empty() {
+ // No specific field requested, this shouldn't happen in our context
+ return Err(ArrowError::InvalidArgumentError(
+ "DataType::Struct case with empty path not supported".to_string()
+ ));
+ }
+
+ // Get the first path element (field name)
+ if let parquet_variant::VariantPathElement::Field { name: field_name } = &path_elements[0] {
+ // Check if the field exists in the shredded structure
+ let struct_array = typed_value.as_any().downcast_ref::<arrow::array::StructArray>()
+ .ok_or_else(|| ArrowError::InvalidArgumentError(
+ "Expected StructArray for DataType::Struct typed_value".to_string()
+ ))?;
+
+ if let Some(_field_column) = struct_array.column_by_name(field_name) {
+ // Field exists in shredded structure - this should be handled by the normal
+ // shredded path traversal, not this fallback case. If we're here, there's
+ // likely an issue with the shredded path logic.
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Field '{}' exists in shredded structure but ended up in struct fallback case",
+ field_name
+ )));
+ } else {
+ // Field does NOT exist in shredded structure
+ // According to shredding spec: check value field for fallback
+
+ let value_field = variant_array.value_field();
+ if value_field.is_none() {
+ // No value field means perfect shredding - missing field = null
+ let num_rows = variant_array.len();
+ return Ok(Arc::new(array::NullArray::new(num_rows)) as ArrayRef);
+ }
Review Comment:
```suggestion
let Some(value_field) = variant_array.value_field() else {
// No value field means perfect shredding - missing field = null
let num_rows = variant_array.len();
return Ok(Arc::new(array::NullArray::new(num_rows)) as ArrayRef);
};
```
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
Review Comment:
Is this random magic constant actually helpful? Why not bigger or smaller?
##########
parquet-variant-compute/src/variant_get/output/variant.rs:
##########
@@ -70,8 +71,81 @@ impl OutputBuilder for VariantOutputBuilder<'_> {
array_builder.append_variant(Variant::from(int_value));
}
}
+ DataType::Struct(_) => {
+ // For shredded objects with struct typed_value, we need sophisticated field access
+ // according to the shredding specification. This handles cases like:
+ // 1. Field exists in shredded structure -> extract from typed_value
+ // 2. Field missing from shredded structure -> fallback to value field or return null
+ // 3. Value field has nulls (perfect shredding) -> return null for missing fields
+
+ use parquet_variant::VariantPath;
+
+ // Extract the field name from the path
+ let path_elements: Vec<_> = self.path.iter().collect();
+ if path_elements.is_empty() {
+ // No specific field requested, this shouldn't happen in our context
+ return Err(ArrowError::InvalidArgumentError(
+ "DataType::Struct case with empty path not supported".to_string()
+ ));
+ }
+
+ // Get the first path element (field name)
+ if let parquet_variant::VariantPathElement::Field { name: field_name } = &path_elements[0] {
+ // Check if the field exists in the shredded structure
+ let struct_array = typed_value.as_any().downcast_ref::<arrow::array::StructArray>()
+ .ok_or_else(|| ArrowError::InvalidArgumentError(
+ "Expected StructArray for DataType::Struct typed_value".to_string()
+ ))?;
+
+ if let Some(_field_column) = struct_array.column_by_name(field_name) {
+ // Field exists in shredded structure - this should be handled by the normal
+ // shredded path traversal, not this fallback case. If we're here, there's
+ // likely an issue with the shredded path logic.
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Field '{}' exists in shredded structure but ended up in struct fallback case",
+ field_name
+ )));
+ } else {
Review Comment:
No need for this `else` (just adds more indentation)
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
Review Comment:
We'll eventually need this parameter, for shredding and unshredding operations that manipulate object fields.
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ }
+ } else {
+ // Non-empty paths: field access functionality
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a path builder that wraps a primitive builder
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ None => {
+ // Create a path builder that wraps a variant array builder
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+#[allow(unused)]
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+#[allow(unused)]
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
+ builder: T,
+ path: VariantPath<'a>,
+}
+
+impl<T: VariantShreddingRowBuilder> VariantShreddingRowBuilder for VariantPathRowBuilder<'_, T> {
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null()
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(v) = value.get_path(&self.path) {
+ self.builder.append_value(&v)
+ } else {
+ self.builder.append_null()?;
+ Ok(false)
+ }
+ }
+ fn finish(&mut self) -> Result<ArrayRef> {
+ self.builder.finish()
+ }
+}
+
+/// Helper trait for converting `Variant` values to arrow primitive values.
+#[allow(unused)]
+trait VariantAsPrimitive<T: ArrowPrimitiveType> {
+ fn as_primitive(&self) -> Option<T::Native>;
+}
+impl VariantAsPrimitive<datatypes::Int32Type> for Variant<'_, '_> {
+ fn as_primitive(&self) -> Option<i32> {
+ self.as_int32()
+ }
+}
+impl VariantAsPrimitive<datatypes::Float64Type> for Variant<'_, '_> {
+ fn as_primitive(&self) -> Option<f64> {
+ self.as_f64()
+ }
+}
+
+/// Builder for shredding variant values to primitive values
+#[allow(unused)]
+struct PrimitiveVariantShreddingRowBuilder<T: ArrowPrimitiveType> {
Review Comment:
another spurious annotation?
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ }
+ } else {
+ // Non-empty paths: field access functionality
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a path builder that wraps a primitive builder
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ None => {
+ // Create a path builder that wraps a variant array builder
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+#[allow(unused)]
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+#[allow(unused)]
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
+ builder: T,
+ path: VariantPath<'a>,
+}
+
+impl<T: VariantShreddingRowBuilder> VariantShreddingRowBuilder for VariantPathRowBuilder<'_, T> {
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null()
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(v) = value.get_path(&self.path) {
+ self.builder.append_value(&v)
+ } else {
+ self.builder.append_null()?;
+ Ok(false)
+ }
+ }
+ fn finish(&mut self) -> Result<ArrayRef> {
+ self.builder.finish()
+ }
+}
+
+/// Helper trait for converting `Variant` values to arrow primitive values.
+#[allow(unused)]
+trait VariantAsPrimitive<T: ArrowPrimitiveType> {
Review Comment:
Isn't this used by the `PrimitiveVariantShreddingRowBuilder` builder at L166 below, which is used by `make_shredding_row_builder` above?
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
Review Comment:
Isn't this used by `shredded_get_path` which is used by `variant_get`?
##########
parquet-variant-compute/src/variant_get/output/struct_output.rs:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray as _, NullBufferBuilder};
+use arrow::datatypes;
+use arrow::datatypes::{ArrowPrimitiveType, FieldRef};
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantObject, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+#[allow(unused)]
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ }
+ } else {
+ // Non-empty paths: field access functionality
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a path builder that wraps a primitive builder
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ None => {
+ // Create a path builder that wraps a variant array builder
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ let path_builder = VariantPathRowBuilder {
+ builder: inner_builder,
+ path,
+ };
+ Ok(Box::new(path_builder))
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+#[allow(unused)]
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+#[allow(unused)]
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
+ builder: T,
+ path: VariantPath<'a>,
+}
+
+impl<T: VariantShreddingRowBuilder> VariantShreddingRowBuilder for VariantPathRowBuilder<'_, T> {
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null()
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(v) = value.get_path(&self.path) {
+ self.builder.append_value(&v)
+ } else {
+ self.builder.append_null()?;
+ Ok(false)
+ }
+ }
+ fn finish(&mut self) -> Result<ArrayRef> {
+ self.builder.finish()
+ }
+}
+
+/// Helper trait for converting `Variant` values to arrow primitive values.
+#[allow(unused)]
+trait VariantAsPrimitive<T: ArrowPrimitiveType> {
+ fn as_primitive(&self) -> Option<T::Native>;
+}
+impl VariantAsPrimitive<datatypes::Int32Type> for Variant<'_, '_> {
+ fn as_primitive(&self) -> Option<i32> {
+ self.as_int32()
+ }
+}
+impl VariantAsPrimitive<datatypes::Float64Type> for Variant<'_, '_> {
+ fn as_primitive(&self) -> Option<f64> {
+ self.as_f64()
+ }
+}
+
+/// Builder for shredding variant values to primitive values
+#[allow(unused)]
+struct PrimitiveVariantShreddingRowBuilder<T: ArrowPrimitiveType> {
+ builder: arrow::array::PrimitiveBuilder<T>,
+}
+
+impl<T> VariantShreddingRowBuilder for PrimitiveVariantShreddingRowBuilder<T>
+where
+ T: ArrowPrimitiveType,
+ for<'m, 'v> Variant<'m, 'v>: VariantAsPrimitive<T>,
+{
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null();
+ Ok(())
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(v) = value.as_primitive() {
+ self.builder.append_value(v);
+ Ok(true)
+ } else {
+ // append null on conversion failure (safe casting behavior)
+ // This matches the default CastOptions::safe = true behavior
+ // TODO: In future steps, respect CastOptions for safe vs unsafe casting
+ self.builder.append_null();
+ Ok(false)
+ }
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ Ok(Arc::new(self.builder.finish()))
+ }
+}
+
+/// Builder for creating VariantArray output (for path extraction without type conversion)
+#[allow(unused)]
+struct VariantArrayShreddingRowBuilder {
+ builder: VariantArrayBuilder,
+}
+
+impl VariantArrayShreddingRowBuilder {
+ fn new(capacity: usize) -> Self {
+ Self {
+ builder: VariantArrayBuilder::new(capacity),
+ }
+ }
+}
+
+impl VariantShreddingRowBuilder for VariantArrayShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null();
+ Ok(())
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ self.builder.append_variant(value.clone());
+ Ok(true)
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ // VariantArrayBuilder::build takes ownership, so we need to replace it
+ let builder = std::mem::replace(&mut self.builder, VariantArrayBuilder::new(0));
+ Ok(Arc::new(builder.build()))
+ }
+}
+
+/// Builder for appending raw binary variant values to a BinaryViewArray. It copies the bytes
+/// as-is, without any decoding.
+#[allow(unused)]
+struct BinaryVariantRowBuilder {
+ nulls: NullBufferBuilder,
+}
+
+impl VariantShreddingRowBuilder for BinaryVariantRowBuilder {
+ fn append_null(&mut self) -> Result<()> {
+ self.nulls.append_null();
+ Ok(())
+ }
+ fn append_value(&mut self, _value: &Variant<'_, '_>) -> Result<bool> {
+ // We need a way to convert a Variant directly to bytes. In particular, we want to just copy
+ // across the underlying value byte slice of a `Variant::Object` or `Variant::List`, without
+ // any interaction with a `VariantMetadata` (because we will just reuse the existing one).
+ //
+ // One could _probably_ emulate this with parquet_variant::VariantBuilder, but it would do a
+ // lot of unnecessary work and would also create a new metadata column we don't need.
+ todo!()
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ // What `finish` does will depend strongly on how `append_value` ends up working. But
+ // ultimately we'll create and return a `VariantArray` instance.
+ todo!()
+ }
+}
+
+/// Builder that extracts a struct. Casting failures produce NULL or error according to options.
+#[allow(unused)]
+struct StructVariantShreddingRowBuilder {
+ nulls: NullBufferBuilder,
+ field_builders: Vec<(FieldRef, Box<dyn VariantShreddingRowBuilder>)>,
+}
+
+impl VariantShreddingRowBuilder for StructVariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()> {
+ for (_, builder) in &mut self.field_builders {
+ builder.append_null()?;
+ }
+ self.nulls.append_null();
+ Ok(())
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ // Casting failure if it's not even an object.
+ let Variant::Object(value) = value else {
+ // TODO: handle casting failure
+ self.append_null()?;
+ return Ok(false);
+ };
+
+ // Process each field. If the field is missing, it becomes NULL. If the field is present,
+ // the child builder handles it from there, and a failed cast could produce NULL or error.
+ //
+ // TODO: This loop costs `O(m lg n)` where `m` is the number of fields in this builder and
+ // `n` is the number of fields in the variant object we're probing. Given that `m` and `n`
+ // could both be large -- indepentently of each other -- we should consider doing something
+ // more clever that bounds the cost to O(m + n).
+ for (field, builder) in &mut self.field_builders {
+ match value.get(field.name()) {
+ None => builder.append_null()?,
+ Some(v) => {
+ builder.append_value(&v)?;
+ }
+ }
+ }
+ self.nulls.append_non_null();
+ Ok(true)
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ let mut fields = Vec::with_capacity(self.field_builders.len());
+ let mut arrays = Vec::with_capacity(self.field_builders.len());
+ for (field, mut builder) in std::mem::take(&mut self.field_builders) {
+ fields.push(field);
+ arrays.push(builder.finish()?);
+ }
+ Ok(Arc::new(arrow::array::StructArray::try_new(
+ fields.into(),
+ arrays,
+ self.nulls.finish(),
+ )?))
+ }
+}
+
+/// Used for actual shredding of binary variant values into shredded variant values
+#[allow(unused)]
+struct ShreddedVariantRowBuilder {
+ metadata: arrow::array::BinaryViewArray,
+ nulls: NullBufferBuilder,
+ value_builder: BinaryVariantRowBuilder,
+ typed_value_builder: Box<dyn VariantShreddingRowBuilder>,
+}
+
+impl VariantShreddingRowBuilder for ShreddedVariantRowBuilder {
+ fn append_null(&mut self) -> Result<()> {
+ self.nulls.append_null();
+ self.value_builder.append_value(&Variant::Null)?;
+ self.typed_value_builder.append_null()
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ self.nulls.append_non_null();
+ if self.typed_value_builder.append_value(value)? {
+ // spec: (value: NULL, typed_value: non-NULL => value is present and shredded)
+ self.value_builder.append_null()?;
+ } else {
+ // spec: (value: non-NULL, typed_value: NULL => value is present and unshredded)
+ self.value_builder.append_value(value)?;
+ }
+ Ok(true)
+ }
+
+ fn finish(&mut self) -> Result<ArrayRef> {
+ let value = self.value_builder.finish()?;
+ let Some(value) = value.as_byte_view_opt() else {
+ return Err(ArrowError::InvalidArgumentError(
+ "Invalid VariantArray: value builder must produce a BinaryViewArray".to_string(),
+ ));
+ };
+ Ok(Arc::new(crate::VariantArray::from_parts(
+ self.metadata.clone(),
+ Some(value.clone()), // TODO: How to consume an ArrayRef directly?
+ Some(self.typed_value_builder.finish()?),
+ self.nulls.finish(),
+ )))
+ }
+}
+
+/// Like VariantShreddingRowBuilder, but for (partially shredded) structs which need special
+/// handling on a per-field basis.
+#[allow(unused)]
+struct VariantShreddingStructRowBuilder {
Review Comment:
As above, probably best added by a follow-on PR that actually uses 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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3265726730
> @alamb My apologies, I have been very occupied with some other things lately, I think it would be best to let @scovich & @liamzwbao to make a new PR with the changes from this PR to speed things up, I would be happy to be an active reviewer or take up any follow up tickets from that PR as things calm down next week.
Sounds like a plan to me
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2291735851
##########
parquet-variant/src/path.rs:
##########
@@ -95,10 +95,19 @@ impl<'a> From<Vec<VariantPathElement<'a>>> for VariantPath<'a> {
}
}
-/// Create from &str
+/// Create from &str with support for dot notation
impl<'a> From<&'a str> for VariantPath<'a> {
fn from(path: &'a str) -> Self {
- VariantPath::new(vec![path.into()])
+ // Support dot notation: "a.x" -> ["a", "x"]
+ if path.contains('.') {
Review Comment:
I agree with the long-form more, but I just added this just as a proof of concept, to see what opinion everyone has of this, can fallback to the original approach, maybe @alamb can also give his 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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2314708756
##########
parquet-variant-compute/src/variant_get/output/row_builder.rs:
##########
@@ -0,0 +1,211 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::ArrayRef;
+use arrow::datatypes;
+use arrow::datatypes::ArrowPrimitiveType;
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ return match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ };
+ }
+
+ // Non-empty paths: field access functionality
+ // Helper macro to reduce duplication when wrapping builders with path functionality
+ macro_rules! wrap_with_path {
+ ($inner_builder:expr) => {
+ Ok(Box::new(VariantPathRowBuilder {
+ builder: $inner_builder,
+ path,
+ }) as Box<dyn VariantShreddingRowBuilder + 'a>)
+ };
+ }
+
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a primitive builder and wrap it with path functionality
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ wrap_with_path!(inner_builder)
+ }
+ None => {
+ // Create a variant array builder and wrap it with path functionality
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ wrap_with_path!(inner_builder)
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
Review Comment:
Sorry for not describing it clearly, the `caller` I described before is the caller of the builder(the logic in `src/variant_get/mod.rs#shredded_get_path`), I commented this because the ` VariantPathRowBuilder` contains a more `path` than the `PrimitiveVariantShreddingRowBuilder`, but the remaining seems the same, the `path` in `VariantPathRowBuilder` will only used in `append_value`, this can be handled in the caller --`shredded_get_path`.
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3262426163
> > I'm pretty sure the actual solution will be to merge the new variant row builder infrastructure in this PR, and then rework `cast_to_variant` to use it. That way, the row builder's constructor can do any column-level transformations that might be needed, before row-oriented visiting begins.
>
> Yes, this sounds like a very plausible approach
>
> > In other words, this PR is probably the most important (= biggest bottleneck) variant PR currently open.
>
> This is my feeling too. @carpecodeum if you don't have time to work on this PR in the next day or two, perhaps @scovich or @liamzwbao could open a new PR (starting with the code from this PR) that we can finish up?
>
> > NOTE: Even tho `variant_get` and `cast_to_variant` would both use the new row builder infra, they are not doing the same thing. The former (when unshredding) needs to use a read-only metadata builder because the `metadata` column already exists; the latter would use a normal metadata builder. So `cast_to_variant` may actually be lower hanging fruit for converting "stuff" to binary variant, with `variant_get` support for the same landing later when we sort out the read-only metadata thing.
>
> 👍
@alamb My apologies, I have been very occupied with some other things lately, I think it would be best to let @scovich & @liamzwbao to make a new PR with the changes from this PR to speed things up, I would be happy to be an active reviewer or take up any follow up tickets from that PR as things calm down next week.
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2291729460
##########
parquet-variant-compute/src/variant_get/output/mod.rs:
##########
@@ -33,6 +34,7 @@ use arrow_schema::{ArrowError, DataType};
/// or as a specific type (e.g. Int32Array).
///
/// See [`instantiate_output_builder`] to create an instance of this trait.
+#[allow(unused)]
pub(crate) trait OutputBuilder {
Review Comment:
You're right that we could potentially simplify this in future PRs
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3255577442
@carpecodeum , @scovich points out that this PR now also has merge conflicts, likely due to merging this
- https://github.com/apache/arrow-rs/pull/8179
> the rebase will have to figure out how to move the newly merged output builder functionality to an appropriate row builder equivalent.
Would you like help doing this? I can make another PR (targeting your branch to do so).
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3217522197
> Just as a heads up I will be out this upcoming week, so I will be much slower on reviews / merging
>
> Please just let me know if anything needs my attention and I'll get to it as quickyl as I can
Thats fine @alamb ! whenever you are available, I think you can provide one final review of this, and we can merge 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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3223551654
Thanks @carpecodeum
THere seems to be some CI failures, can you please resolve them?
<img width="902" height="134" alt="Screenshot 2025-08-26 at 6 18 17 AM" src="https://github.com/user-attachments/assets/557cc8ca-686d-40c2-a61b-57f7a15f4efe" />
@scovich what are your thoughts about merging this PR in and iterating?
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3261703704
> I'm pretty sure the actual solution will be to merge the new variant row builder infrastructure in this PR, and then rework `cast_to_variant` to use it. That way, the row builder's constructor can do any column-level transformations that might be needed, before row-oriented visiting begins.
Yes, this sounds like a very plausible approach
>
> In other words, this PR is probably the most important (= biggest bottleneck) variant PR currently open.
This is my feeling too. @carpecodeum if you don't have time to work on this PR in the next day or two, perhaps @scovich or @liamzwbao could open a new PR (starting with the code from this PR) that we can finish up?
> NOTE: Even tho `variant_get` and `cast_to_variant` would both use the new row builder infra, they are not doing the same thing. The former (when unshredding) needs to use a read-only metadata builder because the `metadata` column already exists; the latter would use a normal metadata builder. So `cast_to_variant` may actually be lower hanging fruit for converting "stuff" to binary variant, with `variant_get` support for the same landing later when we sort out the read-only metadata thing.
👍
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2313754006
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,51 +15,240 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
+ datatypes::Field,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` column and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+ cast_options: &CastOptions,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element is not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ let Some(_value_field) = shredding_state.value_field() else {
Review Comment:
Do you mean this?
```rust
let missing_path_step = || match shredding_state.value_field().is_some() {
true => ShreddedPathStep::NotShredded,
false => ShreddedPathStep::Missing,
};
```
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2314701856
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,51 +15,240 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
+ datatypes::Field,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` column and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+ cast_options: &CastOptions,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element is not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ let Some(_value_field) = shredding_state.value_field() else {
Review Comment:
Yes, I thought the following before, but these two are the same
```
match shredding_state.value_field() {
Some(_) => ShreddedPathStep::Missing,
None => ShreddedPathStep::NotShredded,
};
```
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3211498718
thanks for the review @scovich , i am still resolving most of the comments
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2315998414
##########
parquet-variant-compute/src/variant_get/output/row_builder.rs:
##########
@@ -0,0 +1,211 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::ArrayRef;
+use arrow::datatypes;
+use arrow::datatypes::ArrowPrimitiveType;
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ return match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ };
+ }
+
+ // Non-empty paths: field access functionality
+ // Helper macro to reduce duplication when wrapping builders with path functionality
+ macro_rules! wrap_with_path {
+ ($inner_builder:expr) => {
+ Ok(Box::new(VariantPathRowBuilder {
+ builder: $inner_builder,
+ path,
+ }) as Box<dyn VariantShreddingRowBuilder + 'a>)
+ };
+ }
+
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a primitive builder and wrap it with path functionality
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ wrap_with_path!(inner_builder)
+ }
+ None => {
+ // Create a variant array builder and wrap it with path functionality
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ wrap_with_path!(inner_builder)
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
Review Comment:
Ah. Let's keep an eye out for that -- I originally thought there would be multiple call sites (once we support the other scenarios besides just primitives) and that it would be helpful to factor out the pathing. But if it turns out that there really is just one callsite, then I agree we could eliminate the extra layer of abstraction.
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3268563597
> UPDATE: I started doing some pathfinding on a row-oriented `cast_to_variant`, and it turns out _not_ to need this PR. The _approach_ is the same, but shredding needs variant-to-arrow row builders while the casting needs arrow-to-variant row builders. I'll try to post a PR soon that takes a stab at the latter.
* https://github.com/apache/arrow-rs/pull/8299
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #8166: [Variant] Support Shredded Objects in variant_get
URL: https://github.com/apache/arrow-rs/pull/8166
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3256402566
> @carpecodeum , @scovich points out that this PR now also has merge conflicts, likely due to merging this
>
> * [[Variant] Support typed access for numeric types in variant_get #8179](https://github.com/apache/arrow-rs/pull/8179)
>
> > the rebase will have to figure out how to move the newly merged output builder functionality to an appropriate row builder equivalent.
>
> Would you like help doing this? I can make another PR (targeting your branch to do so).
Hi @alamb @scovich My apologies, I had been quite busy since the past week, I would like to help with the merge conflict resolution, I realise with so many changes going in right now, stuff has become hard to track, including with this PR as well
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3265914456
> > I'm pretty sure the actual solution will be to merge the new variant row builder infrastructure in this PR, and then rework `cast_to_variant` to use it. That way, the row builder's constructor can do any column-level transformations that might be needed, before row-oriented visiting begins.
>
> Yes, this sounds like a very plausible approach
>
> > In other words, this PR is probably the most important (= biggest bottleneck) variant PR currently open.
>
> This is my feeling too. @carpecodeum if you don't have time to work on this PR in the next day or two, perhaps @scovich or @liamzwbao could open a new PR (starting with the code from this PR) that we can finish up?
>
> > NOTE: Even tho `variant_get` and `cast_to_variant` would both use the new row builder infra, they are not doing the same thing. The former (when unshredding) needs to use a read-only metadata builder because the `metadata` column already exists; the latter would use a normal metadata builder. So `cast_to_variant` may actually be lower hanging fruit for converting "stuff" to binary variant, with `variant_get` support for the same landing later when we sort out the read-only metadata thing.
>
> 👍
UPDATE: I started doing some pathfinding on a row-oriented `cast_to_variant`, and it turns out _not_ to need this PR. The _approach_ is the same, but shredding needs variant-to-arrow row builders while the casting needs arrow-to-variant row builders. I'll try to post a PR soon that takes a stab at the latter.
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2311900100
##########
parquet-variant-compute/src/variant_get/output/row_builder.rs:
##########
@@ -0,0 +1,211 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::ArrayRef;
+use arrow::datatypes;
+use arrow::datatypes::ArrowPrimitiveType;
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ return match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ };
+ }
+
+ // Non-empty paths: field access functionality
+ // Helper macro to reduce duplication when wrapping builders with path functionality
+ macro_rules! wrap_with_path {
+ ($inner_builder:expr) => {
+ Ok(Box::new(VariantPathRowBuilder {
+ builder: $inner_builder,
+ path,
+ }) as Box<dyn VariantShreddingRowBuilder + 'a>)
+ };
+ }
+
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a primitive builder and wrap it with path functionality
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ wrap_with_path!(inner_builder)
+ }
+ None => {
+ // Create a variant array builder and wrap it with path functionality
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ wrap_with_path!(inner_builder)
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
Review Comment:
The difference between `VariantPathRowBuilder` and `PrimitiveVariantShreddingRowBuilder` is that `VariantPathRowBuilder` contains a path that is used to retrieve the value in `append_value`. They can be merged if we move the value extraction logic to the caller. Will this become cleaner?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,51 +15,240 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
+ datatypes::Field,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` column and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+ cast_options: &CastOptions,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element is not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ let Some(_value_field) = shredding_state.value_field() else {
Review Comment:
We just want to return different results for different `value_field`, does `match` make it cleaner here?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,51 +15,240 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
+ datatypes::Field,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` column and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+ cast_options: &CastOptions,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element is not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ let Some(_value_field) = shredding_state.value_field() else {
+ return ShreddedPathStep::Missing;
+ };
+ ShreddedPathStep::NotShredded
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ // First, try to downcast to StructArray
+ let Some(struct_array) = typed_value.as_any().downcast_ref::<StructArray>() else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
Review Comment:
If `typed_value` can't be cast to `StructArray`, and `cast_options.safe` is `false`, then we will return an `Err`. Will there be some chance the path located in the `value`, if it is yes, return an `Err` here is expected?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -102,31 +105,57 @@ impl VariantArray {
)));
};
- // Find the value field, if present
- let value = inner
- .column_by_name("value")
- .map(|v| {
- v.as_binary_view_opt().ok_or_else(|| {
- ArrowError::NotYetImplemented(format!(
- "VariantArray 'value' field must be BinaryView, got {}",
- v.data_type()
- ))
- })
- })
- .transpose()?;
-
- // Find the typed_value field, if present
- let typed_value = inner.column_by_name("typed_value");
+ // Extract value and typed_value fields
+ let value = if let Some(value_col) = inner.column_by_name("value") {
+ if let Some(binary_view) = value_col.as_binary_view_opt() {
+ Some(binary_view.clone())
+ } else {
+ return Err(ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ value_col.data_type()
+ )));
+ }
+ } else {
+ None
+ };
+ let typed_value = inner.column_by_name("typed_value").cloned();
// Note these clones are cheap, they just bump the ref count
- let inner = inner.clone();
- let shredding_state =
- ShreddingState::try_new(metadata.clone(), value.cloned(), typed_value.cloned())?;
-
Ok(Self {
+ inner: inner.clone(),
+ metadata: metadata.clone(),
+ shredding_state: ShreddingState::try_new(metadata.clone(), value, typed_value)?,
+ })
+ }
+
+ #[allow(unused)]
+ pub(crate) fn from_parts(
Review Comment:
Do we need to add some doc for this?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -102,31 +105,57 @@ impl VariantArray {
)));
};
- // Find the value field, if present
- let value = inner
- .column_by_name("value")
- .map(|v| {
- v.as_binary_view_opt().ok_or_else(|| {
- ArrowError::NotYetImplemented(format!(
- "VariantArray 'value' field must be BinaryView, got {}",
- v.data_type()
- ))
- })
- })
- .transpose()?;
-
- // Find the typed_value field, if present
- let typed_value = inner.column_by_name("typed_value");
+ // Extract value and typed_value fields
+ let value = if let Some(value_col) = inner.column_by_name("value") {
+ if let Some(binary_view) = value_col.as_binary_view_opt() {
+ Some(binary_view.clone())
+ } else {
+ return Err(ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ value_col.data_type()
+ )));
+ }
+ } else {
+ None
+ };
+ let typed_value = inner.column_by_name("typed_value").cloned();
// Note these clones are cheap, they just bump the ref count
- let inner = inner.clone();
- let shredding_state =
- ShreddingState::try_new(metadata.clone(), value.cloned(), typed_value.cloned())?;
-
Ok(Self {
+ inner: inner.clone(),
+ metadata: metadata.clone(),
+ shredding_state: ShreddingState::try_new(metadata.clone(), value, typed_value)?,
+ })
+ }
+
+ #[allow(unused)]
+ pub(crate) fn from_parts(
+ metadata: BinaryViewArray,
+ value: Option<BinaryViewArray>,
+ typed_value: Option<ArrayRef>,
+ nulls: Option<NullBuffer>,
+ ) -> Self {
+ let mut builder =
+ StructArrayBuilder::new().with_field("metadata", Arc::new(metadata.clone()));
+ if let Some(value) = value.clone() {
+ builder = builder.with_field("value", Arc::new(value));
+ }
+ if let Some(typed_value) = typed_value.clone() {
+ builder = builder.with_field("typed_value", typed_value);
+ }
+ if let Some(nulls) = nulls {
Review Comment:
`value` and `typed_value` both do copy here; do we need to align `null` with them?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -500,4 +668,1013 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_object_field_access() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from("x"));
+ let result = variant_get(&array, options).unwrap();
+
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect x=1
+ assert_eq!(result_variant.value(0), Variant::Int32(1));
+ // Row 1: expect x=42
+ assert_eq!(result_variant.value(1), Variant::Int32(42));
+ }
+
+ /// Test extracting shredded object field with type conversion
+ #[test]
+ fn test_shredded_object_field_as_int32() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as Int32Array (type conversion)
+ let field = Field::new("x", DataType::Int32, false);
+ let options = GetOptions::new_with_path(VariantPath::from("x"))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get Int32Array
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
+ assert_eq!(&result, &expected);
+ }
+
+ /// Helper function to create a shredded variant array representing objects
+ ///
+ /// This creates an array that represents:
+ /// Row 0: {"x": 1, "y": "foo"} (x is shredded, y is in value field)
+ /// Row 1: {"x": 42} (x is shredded, perfect shredding)
+ ///
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains object metadata
+ /// - typed_value: StructArray with field "x" (ShreddedVariantFieldArray)
+ /// - value: contains fallback for unshredded fields like {"y": "foo"}
+ /// - The "x" field has typed_value=Int32Array and value=NULL (perfect shredding)
+ fn shredded_object_with_x_field_variant_array() -> ArrayRef {
+ // Create the base metadata for objects
+ let (metadata, y_field_value) = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("x", Variant::Int32(42));
+ obj.insert("y", Variant::from("foo"));
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ // Create metadata array (same for both rows)
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2));
+
+ // Create the main value field per the 3-step shredding spec:
+ // Step 2: If field not in shredding schema, check value field
+ // Row 0: {"y": "foo"} (y is not shredded, stays in value for step 2)
+ // Row 1: {} (empty object - no unshredded fields)
+ let empty_object_value = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ let value_array = BinaryViewArray::from(vec![
+ Some(y_field_value.as_slice()), // Row 0 has {"y": "foo"}
+ Some(empty_object_value.as_slice()), // Row 1 has {}
+ ]);
+
+ // Create the "x" field as a ShreddedVariantFieldArray
+ // This represents the shredded Int32 values for the "x" field
+ let x_field_typed_value = Int32Array::from(vec![Some(1), Some(42)]);
+
+ // For perfect shredding of the x field, no "value" column, only typed_value
+ let x_field_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(x_field_typed_value))
+ .build();
+
+ // Wrap the x field struct in a ShreddedVariantFieldArray
+ let x_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(x_field_struct))
+ .expect("should create ShreddedVariantFieldArray");
+
+ // Create the main typed_value as a struct containing the "x" field
+ let typed_value_fields = Fields::from(vec![
+ Field::new("x", x_field_shredded.data_type().clone(), true)
+ ]);
+ let typed_value_struct = StructArray::try_new(
+ typed_value_fields,
+ vec![Arc::new(x_field_shredded)],
+ None, // No nulls - both rows have the object structure
+ ).unwrap();
+
+ // Create the main VariantArray
+ let main_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_struct))
+ .build();
+
+ Arc::new(
+ VariantArray::try_new(Arc::new(main_struct)).expect("should create variant array"),
+ )
+ }
+
+ /// Simple test to check if nested paths are supported by current implementation
+ #[test]
+ fn test_simple_nested_path_support() {
Review Comment:
Do we need to assert the expected behavior in this test function?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -500,4 +668,1013 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_object_field_access() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from("x"));
+ let result = variant_get(&array, options).unwrap();
+
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect x=1
+ assert_eq!(result_variant.value(0), Variant::Int32(1));
+ // Row 1: expect x=42
+ assert_eq!(result_variant.value(1), Variant::Int32(42));
+ }
+
+ /// Test extracting shredded object field with type conversion
+ #[test]
+ fn test_shredded_object_field_as_int32() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as Int32Array (type conversion)
+ let field = Field::new("x", DataType::Int32, false);
+ let options = GetOptions::new_with_path(VariantPath::from("x"))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get Int32Array
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
+ assert_eq!(&result, &expected);
+ }
+
+ /// Helper function to create a shredded variant array representing objects
+ ///
+ /// This creates an array that represents:
+ /// Row 0: {"x": 1, "y": "foo"} (x is shredded, y is in value field)
+ /// Row 1: {"x": 42} (x is shredded, perfect shredding)
+ ///
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains object metadata
+ /// - typed_value: StructArray with field "x" (ShreddedVariantFieldArray)
+ /// - value: contains fallback for unshredded fields like {"y": "foo"}
+ /// - The "x" field has typed_value=Int32Array and value=NULL (perfect shredding)
+ fn shredded_object_with_x_field_variant_array() -> ArrayRef {
+ // Create the base metadata for objects
+ let (metadata, y_field_value) = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("x", Variant::Int32(42));
+ obj.insert("y", Variant::from("foo"));
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ // Create metadata array (same for both rows)
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2));
+
+ // Create the main value field per the 3-step shredding spec:
+ // Step 2: If field not in shredding schema, check value field
+ // Row 0: {"y": "foo"} (y is not shredded, stays in value for step 2)
+ // Row 1: {} (empty object - no unshredded fields)
+ let empty_object_value = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ let value_array = BinaryViewArray::from(vec![
+ Some(y_field_value.as_slice()), // Row 0 has {"y": "foo"}
+ Some(empty_object_value.as_slice()), // Row 1 has {}
+ ]);
+
+ // Create the "x" field as a ShreddedVariantFieldArray
+ // This represents the shredded Int32 values for the "x" field
+ let x_field_typed_value = Int32Array::from(vec![Some(1), Some(42)]);
+
+ // For perfect shredding of the x field, no "value" column, only typed_value
+ let x_field_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(x_field_typed_value))
+ .build();
+
+ // Wrap the x field struct in a ShreddedVariantFieldArray
+ let x_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(x_field_struct))
+ .expect("should create ShreddedVariantFieldArray");
+
+ // Create the main typed_value as a struct containing the "x" field
+ let typed_value_fields = Fields::from(vec![
+ Field::new("x", x_field_shredded.data_type().clone(), true)
+ ]);
+ let typed_value_struct = StructArray::try_new(
+ typed_value_fields,
+ vec![Arc::new(x_field_shredded)],
+ None, // No nulls - both rows have the object structure
+ ).unwrap();
+
+ // Create the main VariantArray
+ let main_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_struct))
+ .build();
+
+ Arc::new(
+ VariantArray::try_new(Arc::new(main_struct)).expect("should create variant array"),
+ )
+ }
+
+ /// Simple test to check if nested paths are supported by current implementation
+ #[test]
+ fn test_simple_nested_path_support() {
+ // Check: How does VariantPath parse different strings?
+ println!("Testing path parsing:");
+
+ let path_x = VariantPath::from("x");
+ let elements_x: Vec<_> = path_x.iter().collect();
+ println!(" 'x' -> {} elements: {:?}", elements_x.len(), elements_x);
+
+ let path_ax = VariantPath::from("a.x");
+ let elements_ax: Vec<_> = path_ax.iter().collect();
+ println!(" 'a.x' -> {} elements: {:?}", elements_ax.len(), elements_ax);
+
+ let path_ax_alt = VariantPath::from("$.a.x");
+ let elements_ax_alt: Vec<_> = path_ax_alt.iter().collect();
+ println!(" '$.a.x' -> {} elements: {:?}", elements_ax_alt.len(), elements_ax_alt);
+
+ let path_nested = VariantPath::from("a").join("x");
+ let elements_nested: Vec<_> = path_nested.iter().collect();
+ println!(" VariantPath::from('a').join('x') -> {} elements: {:?}", elements_nested.len(), elements_nested);
+
+ // Use your existing simple test data but try "a.x" instead of "x"
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test if variant_get with REAL nested path throws not implemented error
+ let real_nested_path = VariantPath::from("a").join("x");
+ let options = GetOptions::new_with_path(real_nested_path);
+ let result = variant_get(&array, options);
+
+ match result {
+ Ok(_) => {
+ println!("Nested path 'a.x' works unexpectedly!");
+ },
+ Err(e) => {
+ println!("Nested path 'a.x' error: {}", e);
+ if e.to_string().contains("not yet implemented")
+ || e.to_string().contains("NotYetImplemented") {
+ println!("This is expected - nested paths are not implemented");
+ return;
+ }
+ // Any other error is also expected for now
+ println!("This shows nested paths need implementation");
+ }
+ }
+ }
+
+ /// Test comprehensive variant_get scenarios with Int32 conversion
+ /// Test depth 0: Direct field access "x" with Int32 conversion
+ /// Covers shredded vs non-shredded VariantArrays for simple field access
+ #[test]
+ fn test_depth_0_int32_conversion() {
+ println!("=== Testing Depth 0: Direct field access ===");
+
+ // Non-shredded test data: [{"x": 42}, {"x": "foo"}, {"y": 10}]
+ let unshredded_array = create_depth_0_test_data();
+
+ let field = Field::new("result", DataType::Int32, true);
+ let path = VariantPath::from("x");
+ let options = GetOptions::new_with_path(path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&unshredded_array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![
+ Some(42), // {"x": 42} -> 42
+ None, // {"x": "foo"} -> NULL (type mismatch)
+ None, // {"y": 10} -> NULL (field missing)
+ ]));
+ assert_eq!(&result, &expected);
+ println!("Depth 0 (unshredded) passed");
+
+ // Shredded test data: using simplified approach based on working pattern
+ let shredded_array = create_depth_0_shredded_test_data_simple();
+
+ let field = Field::new("result", DataType::Int32, true);
+ let path = VariantPath::from("x");
+ let options = GetOptions::new_with_path(path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&shredded_array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![
+ Some(42), // {"x": 42} -> 42 (from typed_value)
+ None, // {"x": "foo"} -> NULL (type mismatch, from value field)
+ ]));
+ assert_eq!(&result, &expected);
+ println!("Depth 0 (shredded) passed");
+ }
+
+ /// Test depth 1: Single nested field access "a.x" with Int32 conversion
+ /// Covers shredded vs non-shredded VariantArrays for nested field access
+ #[test]
+ fn test_depth_1_int32_conversion() {
+ println!("=== Testing Depth 1: Single nested field access ===");
+
+ // Non-shredded test data from the GitHub issue
+ let unshredded_array = create_nested_path_test_data();
+
+ let field = Field::new("result", DataType::Int32, true);
+ let path = VariantPath::from("a.x"); // Dot notation!
+ let options = GetOptions::new_with_path(path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&unshredded_array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![
+ Some(55), // {"a": {"x": 55}} -> 55
+ None, // {"a": {"x": "foo"}} -> NULL (type mismatch)
+ ]));
+ assert_eq!(&result, &expected);
+ println!("Depth 1 (unshredded) passed");
+
+ // Shredded test data: depth 1 nested shredding
+ let shredded_array = create_depth_1_shredded_test_data_working();
+
+ let field = Field::new("result", DataType::Int32, true);
+ let path = VariantPath::from("a.x"); // Dot notation!
+ let options = GetOptions::new_with_path(path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&shredded_array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![
+ Some(55), // {"a": {"x": 55}} -> 55 (from nested shredded x)
+ None, // {"a": {"x": "foo"}} -> NULL (type mismatch in nested value)
+ ]));
+ assert_eq!(&result, &expected);
+ println!("Depth 1 (shredded) passed");
+ }
+
+ /// Test depth 2: Double nested field access "a.b.x" with Int32 conversion
+ /// Covers shredded vs non-shredded VariantArrays for deeply nested field access
+ #[test]
+ fn test_depth_2_int32_conversion() {
+ println!("=== Testing Depth 2: Double nested field access ===");
+
+ // Non-shredded test data: [{"a": {"b": {"x": 100}}}, {"a": {"b": {"x": "bar"}}}, {"a": {"b": {"y": 200}}}]
+ let unshredded_array = create_depth_2_test_data();
+
+ let field = Field::new("result", DataType::Int32, true);
+ let path = VariantPath::from("a.b.x"); // Double nested dot notation!
+ let options = GetOptions::new_with_path(path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&unshredded_array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![
+ Some(100), // {"a": {"b": {"x": 100}}} -> 100
+ None, // {"a": {"b": {"x": "bar"}}} -> NULL (type mismatch)
+ None, // {"a": {"b": {"y": 200}}} -> NULL (field missing)
+ ]));
+ assert_eq!(&result, &expected);
+ println!("Depth 2 (unshredded) passed");
+
+ // Shredded test data: depth 2 nested shredding
+ let shredded_array = create_depth_2_shredded_test_data_working();
+
+ let field = Field::new("result", DataType::Int32, true);
+ let path = VariantPath::from("a.b.x"); // Double nested dot notation!
+ let options = GetOptions::new_with_path(path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&shredded_array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![
+ Some(100), // {"a": {"b": {"x": 100}}} -> 100 (from deeply nested shredded x)
+ None, // {"a": {"b": {"x": "bar"}}} -> NULL (type mismatch in deep value)
+ None, // {"a": {"b": {"y": 200}}} -> NULL (field missing in deep structure)
+ ]));
+ assert_eq!(&result, &expected);
+ println!("Depth 2 (shredded) passed");
+ }
+
+ /// Test that demonstrates what CURRENTLY WORKS
+ ///
+ /// This shows that nested path functionality does work, but only when the
+ /// test data matches what the current implementation expects
+ #[test]
+ fn test_current_nested_path_functionality() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field (single level) - this works
+ let single_path = VariantPath::from("x");
+ let field = Field::new("result", DataType::Int32, true);
+ let options = GetOptions::new_with_path(single_path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ println!("Single path 'x' works - result: {:?}", result);
+
+ // Test: Try nested path "a.x" - this is what we need to implement
+ let nested_path = VariantPath::from("a").join("x");
+ let field = Field::new("result", DataType::Int32, true);
+ let options = GetOptions::new_with_path(nested_path)
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ println!("Nested path 'a.x' result: {:?}", result);
+ }
+
+ /// Create test data for depth 0 (direct field access)
+ /// [{"x": 42}, {"x": "foo"}, {"y": 10}]
+ fn create_depth_0_test_data() -> ArrayRef {
+ let mut builder = crate::VariantArrayBuilder::new(3);
+
+ // Row 1: {"x": 42}
+ {
+ let json_str = r#"{"x": 42}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ // Row 2: {"x": "foo"}
+ {
+ let json_str = r#"{"x": "foo"}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ // Row 3: {"y": 10} (missing "x" field)
+ {
+ let json_str = r#"{"y": 10}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ Arc::new(builder.build())
+ }
+
+ /// Create test data for depth 1 (single nested field)
+ /// This represents the exact scenarios from the GitHub issue: "a.x"
+ fn create_nested_path_test_data() -> ArrayRef {
+ let mut builder = crate::VariantArrayBuilder::new(2);
+
+ // Row 1: {"a": {"x": 55}, "b": 42}
+ {
+ let json_str = r#"{"a": {"x": 55}, "b": 42}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ // Row 2: {"a": {"x": "foo"}, "b": 42}
+ {
+ let json_str = r#"{"a": {"x": "foo"}, "b": 42}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ Arc::new(builder.build())
+ }
+
+ /// Create test data for depth 2 (double nested field)
+ /// [{"a": {"b": {"x": 100}}}, {"a": {"b": {"x": "bar"}}}, {"a": {"b": {"y": 200}}}]
+ fn create_depth_2_test_data() -> ArrayRef {
+ let mut builder = crate::VariantArrayBuilder::new(3);
+
+ // Row 1: {"a": {"b": {"x": 100}}}
+ {
+ let json_str = r#"{"a": {"b": {"x": 100}}}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ // Row 2: {"a": {"b": {"x": "bar"}}}
+ {
+ let json_str = r#"{"a": {"b": {"x": "bar"}}}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ // Row 3: {"a": {"b": {"y": 200}}} (missing "x" field)
+ {
+ let json_str = r#"{"a": {"b": {"y": 200}}}"#;
+ let string_array: ArrayRef = Arc::new(StringArray::from(vec![json_str]));
+ if let Ok(variant_array) = json_to_variant(&string_array) {
+ builder.append_variant(variant_array.value(0));
+ } else {
+ builder.append_null();
+ }
+ }
+
+ Arc::new(builder.build())
+ }
+
+ /// Create simple shredded test data for depth 0 using a simplified working pattern
+ /// Creates 2 rows: [{"x": 42}, {"x": "foo"}] with "x" shredded where possible
+ fn create_depth_0_shredded_test_data_simple() -> ArrayRef {
+ // Create base metadata using the working pattern
+ let (metadata, string_x_value) = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("x", Variant::from("foo"));
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ // Metadata array (same for both rows)
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2));
+
+ // Value array following the 3-step shredding spec:
+ // Row 0: {} (x is shredded, no unshredded fields)
+ // Row 1: {"x": "foo"} (x is a string, can't be shredded to Int32)
+ let empty_object_value = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ let value_array = BinaryViewArray::from(vec![
+ Some(empty_object_value.as_slice()), // Row 0: {} (x shredded out)
+ Some(string_x_value.as_slice()), // Row 1: {"x": "foo"} (fallback)
+ ]);
+
+ // Create the "x" field as a ShreddedVariantFieldArray
+ let x_field_typed_value = Int32Array::from(vec![Some(42), None]);
+
+ // For the x field, only typed_value (perfect shredding when possible)
+ let x_field_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(x_field_typed_value))
+ .build();
+
+ let x_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(x_field_struct))
+ .expect("should create ShreddedVariantFieldArray");
+
+ // Create the main typed_value as a struct containing the "x" field
+ let typed_value_fields = Fields::from(vec![
+ Field::new("x", x_field_shredded.data_type().clone(), true)
+ ]);
+ let typed_value_struct = StructArray::try_new(
+ typed_value_fields,
+ vec![Arc::new(x_field_shredded)],
+ None,
+ ).unwrap();
+
+ // Build final VariantArray
+ let struct_array = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_struct))
+ .build();
+
+ Arc::new(
+ VariantArray::try_new(Arc::new(struct_array)).expect("should create VariantArray"),
+ )
+ }
+
+ /// Create working depth 1 shredded test data based on the existing working pattern
+ /// This creates a properly structured shredded variant for "a.x" where:
+ /// - Row 0: {"a": {"x": 55}, "b": 42} with a.x shredded into typed_value
+ /// - Row 1: {"a": {"x": "foo"}, "b": 42} with a.x fallback to value field due to type mismatch
+ fn create_depth_1_shredded_test_data_working() -> ArrayRef {
+ // Create metadata following the working pattern from shredded_object_with_x_field_variant_array
+ let (metadata, _) = {
+ // Create nested structure: {"a": {"x": 55}, "b": 42}
+ let a_variant = {
+ let mut a_builder = parquet_variant::VariantBuilder::new();
+ let mut a_obj = a_builder.new_object();
+ a_obj.insert("x", Variant::Int32(55)); // "a.x" field (shredded when possible)
+ a_obj.finish().unwrap()
+ };
+
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("a", a_variant);
+ obj.insert("b", Variant::Int32(42));
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2));
+
+ // Create value arrays for the fallback case
+ // Following the spec: if field cannot be shredded, it stays in value
+ let empty_object_value = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ // Row 1 fallback: use the working pattern from the existing shredded test
+ // This avoids metadata issues by using the simple fallback approach
+ let row1_fallback = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("fallback", Variant::from("data"));
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ let value_array = BinaryViewArray::from(vec![
+ Some(empty_object_value.as_slice()), // Row 0: {} (everything shredded except b in unshredded fields)
+ Some(row1_fallback.as_slice()), // Row 1: {"a": {"x": "foo"}, "b": 42} (a.x can't be shredded)
+ ]);
+
+ // Create the nested shredded structure
+ // Level 2: x field (the deepest level)
+ let x_typed_value = Int32Array::from(vec![Some(55), None]);
+ let x_field_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(x_typed_value))
+ .build();
+ let x_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(x_field_struct))
+ .expect("should create ShreddedVariantFieldArray for x");
+
+ // Level 1: a field containing x field + value field for fallbacks
+ // The "a" field needs both typed_value (for shredded x) and value (for fallback cases)
+
+ // Create the value field for "a" (for cases where a.x can't be shredded)
+ let a_value_data = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+ let a_value_array = BinaryViewArray::from(vec![
+ None, // Row 0: x is shredded, so no value fallback needed
+ Some(a_value_data.as_slice()), // Row 1: fallback for a.x="foo" (but logic will check typed_value first)
+ ]);
+
+ let a_inner_fields = Fields::from(vec![
+ Field::new("x", x_field_shredded.data_type().clone(), true)
+ ]);
+ let a_inner_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(StructArray::try_new(
+ a_inner_fields,
+ vec![Arc::new(x_field_shredded)],
+ None,
+ ).unwrap()))
+ .with_field("value", Arc::new(a_value_array))
+ .build();
+ let a_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(a_inner_struct))
+ .expect("should create ShreddedVariantFieldArray for a");
+
+ // Level 0: main typed_value struct containing a field
+ let typed_value_fields = Fields::from(vec![
+ Field::new("a", a_field_shredded.data_type().clone(), true)
+ ]);
+ let typed_value_struct = StructArray::try_new(
+ typed_value_fields,
+ vec![Arc::new(a_field_shredded)],
+ None,
+ ).unwrap();
+
+ // Build final VariantArray
+ let struct_array = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_struct))
+ .build();
+
+ Arc::new(
+ VariantArray::try_new(Arc::new(struct_array)).expect("should create VariantArray"),
+ )
+ }
+
+ /// Create working depth 2 shredded test data for "a.b.x" paths
+ /// This creates a 3-level nested shredded structure where:
+ /// - Row 0: {"a": {"b": {"x": 100}}} with a.b.x shredded into typed_value
+ /// - Row 1: {"a": {"b": {"x": "bar"}}} with type mismatch fallback
+ /// - Row 2: {"a": {"b": {"y": 200}}} with missing field fallback
+ fn create_depth_2_shredded_test_data_working() -> ArrayRef {
+ // Create metadata following the working pattern
+ let (metadata, _) = {
+ // Create deeply nested structure: {"a": {"b": {"x": 100}}}
+ let b_variant = {
+ let mut b_builder = parquet_variant::VariantBuilder::new();
+ let mut b_obj = b_builder.new_object();
+ b_obj.insert("x", Variant::Int32(100));
+ b_obj.finish().unwrap()
+ };
+
+ let a_variant = {
+ let mut a_builder = parquet_variant::VariantBuilder::new();
+ let mut a_obj = a_builder.new_object();
+ a_obj.insert("b", b_variant);
+ a_obj.finish().unwrap()
+ };
+
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("a", a_variant); // "a" field containing b
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 3));
+
+ // Create value arrays for fallback cases
+ let empty_object_value = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ // Simple fallback values - avoiding complex nested metadata
+ let value_array = BinaryViewArray::from(vec![
+ Some(empty_object_value.as_slice()), // Row 0: fully shredded
+ Some(empty_object_value.as_slice()), // Row 1: fallback (simplified)
+ Some(empty_object_value.as_slice()), // Row 2: fallback (simplified)
+ ]);
+
+ // Create the deeply nested shredded structure: a.b.x
+
+ // Level 3: x field (deepest level)
+ let x_typed_value = Int32Array::from(vec![Some(100), None, None]);
+ let x_field_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(x_typed_value))
+ .build();
+ let x_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(x_field_struct))
+ .expect("should create ShreddedVariantFieldArray for x");
+
+ // Level 2: b field containing x field + value field
+ let b_value_data = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+ let b_value_array = BinaryViewArray::from(vec![
+ None, // Row 0: x is shredded
+ Some(b_value_data.as_slice()), // Row 1: fallback for b.x="bar"
+ Some(b_value_data.as_slice()), // Row 2: fallback for b.y=200
+ ]);
+
+ let b_inner_fields = Fields::from(vec![
+ Field::new("x", x_field_shredded.data_type().clone(), true)
+ ]);
+ let b_inner_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(StructArray::try_new(
+ b_inner_fields,
+ vec![Arc::new(x_field_shredded)],
+ None,
+ ).unwrap()))
+ .with_field("value", Arc::new(b_value_array))
+ .build();
+ let b_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(b_inner_struct))
+ .expect("should create ShreddedVariantFieldArray for b");
+
+ // Level 1: a field containing b field + value field
+ let a_value_data = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+ let a_value_array = BinaryViewArray::from(vec![
+ None, // Row 0: b is shredded
+ Some(a_value_data.as_slice()), // Row 1: fallback for a.b.*
+ Some(a_value_data.as_slice()), // Row 2: fallback for a.b.*
+ ]);
+
+ let a_inner_fields = Fields::from(vec![
+ Field::new("b", b_field_shredded.data_type().clone(), true)
+ ]);
+ let a_inner_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(StructArray::try_new(
+ a_inner_fields,
+ vec![Arc::new(b_field_shredded)],
+ None,
+ ).unwrap()))
+ .with_field("value", Arc::new(a_value_array))
+ .build();
+ let a_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(a_inner_struct))
+ .expect("should create ShreddedVariantFieldArray for a");
+
+ // Level 0: main typed_value struct containing a field
+ let typed_value_fields = Fields::from(vec![
+ Field::new("a", a_field_shredded.data_type().clone(), true)
+ ]);
+ let typed_value_struct = StructArray::try_new(
+ typed_value_fields,
+ vec![Arc::new(a_field_shredded)],
+ None,
+ ).unwrap();
+
+ // Build final VariantArray
+ let struct_array = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_struct))
+ .build();
+
+ Arc::new(
+ VariantArray::try_new(Arc::new(struct_array)).expect("should create VariantArray"),
+ )
+ }
+
+ #[test]
+ fn test_strict_cast_options_downcast_failure() {
+ use arrow::error::ArrowError;
+ use arrow::compute::CastOptions;
+ use arrow::datatypes::{DataType, Field};
+ use std::sync::Arc;
+ use parquet_variant::VariantPath;
+
+ // Use the existing simple test data that has Int32 as typed_value
+ let variant_array = perfectly_shredded_int32_variant_array();
+
+ // Try to access a field with safe cast options (should return NULLs)
+ let safe_options = GetOptions {
+ path: VariantPath::from("nonexistent_field"),
+ as_type: Some(Arc::new(Field::new("result", DataType::Int32, true))),
+ cast_options: CastOptions::default(), // safe = true
+ };
+
+ let variant_array_ref: Arc<dyn arrow::array::Array> = variant_array.clone();
+ let result = variant_get(&variant_array_ref, safe_options);
+ // Should succeed and return NULLs (safe behavior)
+ assert!(result.is_ok());
+ let result_array = result.unwrap();
+ assert_eq!(result_array.len(), 3);
+ assert!(result_array.is_null(0));
+ assert!(result_array.is_null(1));
+ assert!(result_array.is_null(2));
+
+ // Try to access a field with strict cast options (should error)
+ let strict_options = GetOptions {
+ path: VariantPath::from("nonexistent_field"),
+ as_type: Some(Arc::new(Field::new("result", DataType::Int32, true))),
+ cast_options: CastOptions { safe: false, ..Default::default() },
+ };
+
+ let result = variant_get(&variant_array_ref, strict_options);
+ // Should fail with a cast error
+ assert!(result.is_err());
+ let error = result.unwrap_err();
+ assert!(matches!(error, ArrowError::CastError(_)));
+ assert!(error.to_string().contains("Cannot access field 'nonexistent_field' on non-struct type"));
+ }
+
+ #[test]
+ fn test_null_buffer_union_for_shredded_paths() {
+ use arrow::compute::CastOptions;
Review Comment:
Do we need to add another row where path `a.x` does not exist
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2314701856
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,51 +15,240 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
+ datatypes::Field,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` column and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+ cast_options: &CastOptions,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element is not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ let Some(_value_field) = shredding_state.value_field() else {
Review Comment:
Yes,
```
match shredding_state.value_field() {
Some(_) => ShreddedPathStep::Missing,
None => ShreddedPathStep::NotShredded,
};
```
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2313774965
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -15,51 +15,240 @@
// specific language governing permissions and limitations
// under the License.
use arrow::{
- array::{Array, ArrayRef},
+ array::{self, Array, ArrayRef, BinaryViewArray, StructArray},
compute::CastOptions,
+ datatypes::Field,
error::Result,
};
-use arrow_schema::{ArrowError, FieldRef};
-use parquet_variant::VariantPath;
+use arrow_schema::{ArrowError, DataType, FieldRef};
+use parquet_variant::{VariantPath, VariantPathElement};
use crate::variant_array::ShreddingState;
-use crate::variant_get::output::instantiate_output_builder;
-use crate::VariantArray;
+use crate::{variant_array::ShreddedVariantFieldArray, VariantArray};
+
+use std::sync::Arc;
mod output;
+pub(crate) enum ShreddedPathStep<'a> {
+ /// Path step succeeded, return the new shredding state
+ Success(&'a ShreddingState),
+ /// The path element is not present in the `typed_value` column and there is no `value` column,
+ /// so we we know it does not exist. It, and all paths under it, are all-NULL.
+ Missing,
+ /// The path element is not present in the `typed_value` column and must be retrieved from the `value`
+ /// column instead. The caller should be prepared to handle any value, including the requested
+ /// type, an arbitrary "wrong" type, or `Variant::Null`.
+ NotShredded,
+}
+
+/// Given a shredded variant field -- a `(value?, typed_value?)` pair -- try to take one path step
+/// deeper. For a `VariantPathElement::Field`, the step fails if there is no `typed_value` at this
+/// level, or if `typed_value` is not a struct, or if the requested field name does not exist.
+///
+/// TODO: Support `VariantPathElement::Index`? It wouldn't be easy, and maybe not even possible.
+pub(crate) fn follow_shredded_path_element<'a>(
+ shredding_state: &'a ShreddingState,
+ path_element: &VariantPathElement<'_>,
+ cast_options: &CastOptions,
+) -> Result<ShreddedPathStep<'a>> {
+ // If the requested path element is not present in `typed_value`, and `value` is missing, then
+ // we know it does not exist; it, and all paths under it, are all-NULL.
+ let missing_path_step = || {
+ let Some(_value_field) = shredding_state.value_field() else {
+ return ShreddedPathStep::Missing;
+ };
+ ShreddedPathStep::NotShredded
+ };
+
+ let Some(typed_value) = shredding_state.typed_value_field() else {
+ return Ok(missing_path_step());
+ };
+
+ match path_element {
+ VariantPathElement::Field { name } => {
+ // Try to step into the requested field name of a struct.
+ // First, try to downcast to StructArray
+ let Some(struct_array) = typed_value.as_any().downcast_ref::<StructArray>() else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
Review Comment:
Good catch. This scenario could happen if e.g. we asked for `v:a.b.c::INT` and `v.typed_value.a.typed_value` is shredded as something other than struct. That's not an error at all, there's no rule that the path we asked for has to match the shredding of the underlying data. It just means we need to fetch the value from `v.typed_value.a.value` instead.
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2323365784
##########
parquet-variant-compute/src/variant_get/output/row_builder.rs:
##########
@@ -0,0 +1,211 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::ArrayRef;
+use arrow::datatypes;
+use arrow::datatypes::ArrowPrimitiveType;
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
Review Comment:
could be removed
```suggestion
```
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -500,4 +668,1013 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_object_field_access() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from("x"));
+ let result = variant_get(&array, options).unwrap();
+
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect x=1
+ assert_eq!(result_variant.value(0), Variant::Int32(1));
+ // Row 1: expect x=42
+ assert_eq!(result_variant.value(1), Variant::Int32(42));
+ }
+
+ /// Test extracting shredded object field with type conversion
+ #[test]
+ fn test_shredded_object_field_as_int32() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as Int32Array (type conversion)
+ let field = Field::new("x", DataType::Int32, false);
+ let options = GetOptions::new_with_path(VariantPath::from("x"))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get Int32Array
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
+ assert_eq!(&result, &expected);
+ }
+
+ /// Helper function to create a shredded variant array representing objects
+ ///
+ /// This creates an array that represents:
+ /// Row 0: {"x": 1, "y": "foo"} (x is shredded, y is in value field)
+ /// Row 1: {"x": 42} (x is shredded, perfect shredding)
+ ///
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains object metadata
+ /// - typed_value: StructArray with field "x" (ShreddedVariantFieldArray)
+ /// - value: contains fallback for unshredded fields like {"y": "foo"}
+ /// - The "x" field has typed_value=Int32Array and value=NULL (perfect shredding)
+ fn shredded_object_with_x_field_variant_array() -> ArrayRef {
+ // Create the base metadata for objects
+ let (metadata, y_field_value) = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("x", Variant::Int32(42));
+ obj.insert("y", Variant::from("foo"));
+ obj.finish().unwrap();
+ builder.finish()
+ };
+
+ // Create metadata array (same for both rows)
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2));
+
+ // Create the main value field per the 3-step shredding spec:
+ // Step 2: If field not in shredding schema, check value field
+ // Row 0: {"y": "foo"} (y is not shredded, stays in value for step 2)
+ // Row 1: {} (empty object - no unshredded fields)
+ let empty_object_value = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let obj = builder.new_object();
+ obj.finish().unwrap();
+ let (_, value) = builder.finish();
+ value
+ };
+
+ let value_array = BinaryViewArray::from(vec![
+ Some(y_field_value.as_slice()), // Row 0 has {"y": "foo"}
+ Some(empty_object_value.as_slice()), // Row 1 has {}
+ ]);
+
+ // Create the "x" field as a ShreddedVariantFieldArray
+ // This represents the shredded Int32 values for the "x" field
+ let x_field_typed_value = Int32Array::from(vec![Some(1), Some(42)]);
+
+ // For perfect shredding of the x field, no "value" column, only typed_value
+ let x_field_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("typed_value", Arc::new(x_field_typed_value))
+ .build();
+
+ // Wrap the x field struct in a ShreddedVariantFieldArray
+ let x_field_shredded = ShreddedVariantFieldArray::try_new(Arc::new(x_field_struct))
+ .expect("should create ShreddedVariantFieldArray");
+
+ // Create the main typed_value as a struct containing the "x" field
+ let typed_value_fields = Fields::from(vec![
+ Field::new("x", x_field_shredded.data_type().clone(), true)
+ ]);
+ let typed_value_struct = StructArray::try_new(
+ typed_value_fields,
+ vec![Arc::new(x_field_shredded)],
+ None, // No nulls - both rows have the object structure
+ ).unwrap();
+
+ // Create the main VariantArray
+ let main_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_struct))
+ .build();
+
+ Arc::new(
+ VariantArray::try_new(Arc::new(main_struct)).expect("should create variant array"),
+ )
+ }
+
+ /// Simple test to check if nested paths are supported by current implementation
+ #[test]
+ fn test_simple_nested_path_support() {
Review Comment:
yes!
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -388,43 +525,720 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
Review Comment:
This is a great set of ideas, but perhaps one we can do as a follow on PR
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3242867448
Thank you for your reviews @klion26 and @scovich I will work on the suggestions
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3260318512
Attn @alamb @carpecodeum @liamzwbao :
One thing I noticed today --
* while making an early pass over https://github.com/apache/arrow-rs/pull/8241
* and re-reviewing https://github.com/apache/arrow-rs/pull/8235
I noticed that `cast_to_variant` follows the same problematic column-oriented approach as the output builder this PR replaces with a row builder. In particular, it converts entire columns to variant, which means repeatedly re-encoding deeply nested variant values, transferring their bytes to new arrays, and recreating incomplete metadata columns that immediately get thrown away.
I spent some time today, trying to refactor it to be row-oriented (while keeping a similar code structure), but late in the process I realized that several array types break it because they need to do some holistic column-level transformation, which cannot be captured efficiently by a row-wise approach:
* maps - cast the key column to string
* dictionaries - convert the dictionary value column and then clone values from it as needed
* run-end encoding - similar to dictionary, but _also_ need to convert the incoming logical index to physical run index, which is a linear search in row-oriented code (= quadratic cost!)
I'm pretty sure the actual solution will be to merge the new variant row builder infrastructure in this PR, and then rework `cast_to_variant` to use it. That way, the row builder's constructor can do any column-level transformations that might be needed, before row-oriented visiting begins.
In other words, this PR is probably the most important (= biggest bottleneck) variant PR currently open.
NOTE: Even tho `variant_get` and `cast_to_variant` would both use the new row builder infra, they are not doing the same thing. The former (when unshredding) needs to use a read-only metadata builder because the `metadata` column already exists; the latter would use a normal metadata builder. So `cast_to_variant` may actually be lower hanging fruit for converting "stuff" to binary variant, with `variant_get` support for the same landing later when we sort out the read-only metadata thing.
Thoughts?
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2298323189
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -102,31 +105,57 @@ impl VariantArray {
)));
};
- // Find the value field, if present
- let value = inner
- .column_by_name("value")
- .map(|v| {
- v.as_binary_view_opt().ok_or_else(|| {
- ArrowError::NotYetImplemented(format!(
- "VariantArray 'value' field must be BinaryView, got {}",
- v.data_type()
- ))
- })
- })
- .transpose()?;
-
- // Find the typed_value field, if present
- let typed_value = inner.column_by_name("typed_value");
+ // Extract value and typed_value fields
+ let value = if let Some(value_col) = inner.column_by_name("value") {
+ if let Some(binary_view) = value_col.as_binary_view_opt() {
+ Some(binary_view.clone())
+ } else {
+ return Err(ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ value_col.data_type()
+ )));
+ }
+ } else {
+ None
+ };
Review Comment:
tiny nit?
```suggestion
let value = match inner.column_by_name("value") {
Some(value_col) => match value_col.as_binary_view_opt() {
Some(binary_view) => Some(binary_view.clone()),
None => return Err(ArrowError::NotYetImplemented(format!(
"VariantArray 'value' field must be BinaryView, got {}",
value_col.data_type()
))),
}
None => None,
})
```
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -166,28 +195,28 @@ impl VariantArray {
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {
- ShreddingState::Unshredded { metadata, value } => {
- Variant::new(metadata.value(index), value.value(index))
+ ShreddingState::Unshredded { value, .. } => {
+ // Unshredded case
+ Variant::new(self.metadata.value(index), value.value(index))
}
ShreddingState::Typed { typed_value, .. } => {
+ // Typed case (formerly PerfectlyShredded)
if typed_value.is_null(index) {
Variant::Null
} else {
typed_value_to_variant(typed_value, index)
}
}
- ShreddingState::PartiallyShredded {
- metadata,
- value,
- typed_value,
- } => {
+ ShreddingState::PartiallyShredded { value, typed_value, .. } => {
+ // PartiallyShredded case (formerly ImperfectlyShredded)
Review Comment:
This is incorrect naming. The term "partially shredded" is specifically defined in the [variant shredding spec](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#value-shredding), and that definition does not agree with how the current code uses the name:
> An object is _partially shredded_ when the `value` is an object and the `typed_value` is a shredded object.
Partial shredding is just one specific kind of imperfect shredding -- _any_ type can shred imperfectly, producing both `value` and `typed_value` columns.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -213,6 +242,166 @@ impl VariantArray {
}
}
+/// One shredded field of a partially or prefectly shredded variant. For example, suppose the
+/// shredding schema for variant `v` treats it as an object with a single field `a`, where `a` is
+/// itself a struct with the single field `b` of type INT. Then the physical layout of the column
+/// is:
+///
+/// ```text
+/// v: VARIANT {
+/// metadata: BINARY,
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
+/// value: BINARY,
+/// typed_value: INT,
+/// },
+/// },
+/// },
+/// },
+/// }
+/// ```
+///
+/// In the above, each row of `v.value` is either a variant value (shredding failed, `v` was not an
+/// object at all) or a variant object (partial shredding, `v` was an object but included unexpected
+/// fields other than `a`), or is NULL (perfect shredding, `v` was an object containing only the
+/// single expected field `a`).
+///
+/// A similar story unfolds for each `v.typed_value.a.value` -- a variant value if shredding failed
+/// (`v:a` was not an object at all), or a variant object (`v:a` was an object with unexpected
+/// additional fields), or NULL (`v:a` was an object containing only the single expected field `b`).
+///
+/// Finally, `v.typed_value.a.typed_value.b.value` is either NULL (`v:a.b` was an integer) or else a
+/// variant value (which could be `Variant::Null`).
+#[derive(Debug)]
+pub struct ShreddedVariantFieldArray {
+ /// Reference to the underlying StructArray
+ inner: StructArray,
+ shredding_state: ShreddingState,
+}
+
+#[allow(unused)]
+impl ShreddedVariantFieldArray {
+ /// Creates a new `ShreddedVariantFieldArray` from a [`StructArray`].
+ ///
+ /// # Arguments
+ /// - `inner` - The underlying [`StructArray`] that contains the variant data.
+ ///
+ /// # Returns
+ /// - A new instance of `ShreddedVariantFieldArray`.
+ ///
+ /// # Errors:
+ /// - If the `StructArray` does not contain the required fields
+ ///
+ /// # Requirements of the `StructArray`
+ ///
+ /// 1. An optional field named `value` that is binary, large_binary, or
+ /// binary_view
+ ///
+ /// 2. An optional field named `typed_value` which can be any primitive type
+ /// or be a list, large_list, list_view or struct
+ ///
+ /// Currently, only `value` columns of type [`BinaryViewArray`] are supported.
+ pub fn try_new(inner: ArrayRef) -> Result<Self, ArrowError> {
+ let Some(inner_struct) = inner.as_struct_opt() else {
+ return Err(ArrowError::InvalidArgumentError(
+ "Invalid ShreddedVariantFieldArray: requires StructArray as input".to_string(),
+ ));
+ };
+
+ // Extract value and typed_value fields (metadata is not expected in ShreddedVariantFieldArray)
+ let value = inner_struct.column_by_name("value").and_then(|col| col.as_binary_view_opt().cloned());
+ let typed_value = inner_struct.column_by_name("typed_value").cloned();
+
+ // Use a dummy metadata for the constructor (ShreddedVariantFieldArray doesn't have metadata)
+ let dummy_metadata = arrow::array::BinaryViewArray::new_null(inner_struct.len());
Review Comment:
```suggestion
let dummy_metadata = BinaryViewArray::new_null(inner_struct.len());
```
##########
parquet-variant-compute/src/variant_get/output/mod.rs:
##########
@@ -15,80 +15,4 @@
// specific language governing permissions and limitations
// under the License.
-mod primitive;
-mod variant;
-
-use crate::variant_get::output::primitive::PrimitiveOutputBuilder;
-use crate::variant_get::output::variant::VariantOutputBuilder;
-use crate::variant_get::GetOptions;
-use crate::VariantArray;
-use arrow::array::{ArrayRef, BinaryViewArray};
-use arrow::datatypes::Int32Type;
-use arrow::error::Result;
-use arrow_schema::{ArrowError, DataType};
-
-/// This trait represents something that gets the output of the variant_get kernel.
-///
-/// For example, there are specializations for writing the output as a VariantArray,
-/// or as a specific type (e.g. Int32Array).
-///
-/// See [`instantiate_output_builder`] to create an instance of this trait.
-pub(crate) trait OutputBuilder {
- /// create output for a shredded variant array
- fn partially_shredded(
- &self,
- variant_array: &VariantArray,
- metadata: &BinaryViewArray,
- value_field: &BinaryViewArray,
- typed_value: &ArrayRef,
- ) -> Result<ArrayRef>;
-
- /// output for a perfectly shredded variant array
- fn typed(
- &self,
- variant_array: &VariantArray,
- metadata: &BinaryViewArray,
- typed_value: &ArrayRef,
- ) -> Result<ArrayRef>;
-
- /// write out an unshredded variant array
- fn unshredded(
- &self,
- variant_array: &VariantArray,
- metadata: &BinaryViewArray,
- value_field: &BinaryViewArray,
- ) -> Result<ArrayRef>;
-
- /// write out an all-null variant array
- fn all_null(
- &self,
- variant_array: &VariantArray,
- metadata: &BinaryViewArray,
- ) -> Result<ArrayRef>;
-}
-
-pub(crate) fn instantiate_output_builder<'a>(
- options: GetOptions<'a>,
-) -> Result<Box<dyn OutputBuilder + 'a>> {
- let GetOptions {
- as_type,
- path,
- cast_options,
- } = options;
-
- let Some(as_type) = as_type else {
- return Ok(Box::new(VariantOutputBuilder::new(path)));
- };
-
- // handle typed output
- match as_type.data_type() {
- DataType::Int32 => Ok(Box::new(PrimitiveOutputBuilder::<Int32Type>::new(
- path,
- as_type,
- cast_options,
- ))),
- dt => Err(ArrowError::NotYetImplemented(format!(
- "variant_get with as_type={dt} is not implemented yet",
- ))),
- }
-}
+pub(crate) mod row_builder;
Review Comment:
Shouldn't cargo fmt have fixed this?
##########
parquet-variant/src/path.rs:
##########
@@ -95,10 +95,10 @@ impl<'a> From<Vec<VariantPathElement<'a>>> for VariantPath<'a> {
}
}
-/// Create from &str
+/// Create from &str with support for dot notation
impl<'a> From<&'a str> for VariantPath<'a> {
Review Comment:
Rescuing https://github.com/apache/arrow-rs/pull/8166#discussion_r2286247849 from github oblivion:
> > This is dangerous without some well-defined way to escape values. Otherwise, a column name that contains a dot is perfectly legal but would produce an incorrect path.
>
> I agree with the long-form more, but I just added this just as a proof of concept, to see what opinion everyone has of this, can fallback to the original approach, maybe @alamb can also give his opinion?
(impl From makes it part of the public API, and I'm nervious about giving people a massive footgun)
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -166,28 +195,28 @@ impl VariantArray {
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {
- ShreddingState::Unshredded { metadata, value } => {
- Variant::new(metadata.value(index), value.value(index))
+ ShreddingState::Unshredded { value, .. } => {
+ // Unshredded case
+ Variant::new(self.metadata.value(index), value.value(index))
}
ShreddingState::Typed { typed_value, .. } => {
+ // Typed case (formerly PerfectlyShredded)
Review Comment:
Aside:
* https://github.com/apache/parquet-format/pull/520
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -166,28 +195,28 @@ impl VariantArray {
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {
- ShreddingState::Unshredded { metadata, value } => {
- Variant::new(metadata.value(index), value.value(index))
+ ShreddingState::Unshredded { value, .. } => {
+ // Unshredded case
+ Variant::new(self.metadata.value(index), value.value(index))
}
ShreddingState::Typed { typed_value, .. } => {
+ // Typed case (formerly PerfectlyShredded)
if typed_value.is_null(index) {
Variant::Null
} else {
typed_value_to_variant(typed_value, index)
}
}
- ShreddingState::PartiallyShredded {
- metadata,
- value,
- typed_value,
- } => {
+ ShreddingState::PartiallyShredded { value, typed_value, .. } => {
+ // PartiallyShredded case (formerly ImperfectlyShredded)
if typed_value.is_null(index) {
- Variant::new(metadata.value(index), value.value(index))
+ Variant::new(self.metadata.value(index), value.value(index))
} else {
typed_value_to_variant(typed_value, index)
}
}
ShreddingState::AllNull { .. } => {
Review Comment:
Another naming nit: This should probably be called `ShreddingState::Missing`, to match terminology of the [shredding spec](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#value-shredding)?
<img width="668" height="90" alt="image" src="https://github.com/user-attachments/assets/9faf4d67-b170-4e75-9ea9-d96538d7a7f6" />
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -166,28 +195,28 @@ impl VariantArray {
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {
- ShreddingState::Unshredded { metadata, value } => {
- Variant::new(metadata.value(index), value.value(index))
+ ShreddingState::Unshredded { value, .. } => {
+ // Unshredded case
+ Variant::new(self.metadata.value(index), value.value(index))
}
ShreddingState::Typed { typed_value, .. } => {
+ // Typed case (formerly PerfectlyShredded)
Review Comment:
Not a fan of `Typed` as the name here. It doesn't match any language in the variant shredding spec, and IMO it doesn't really self-describe either. At a minimum we should consider `StronglyTyped`? (in contrast to variant normally being weakly typed)?
But I'm curious why "perfectly shredded" and "imperfectly shredded" are unclear or otherwise unwelcome? (see comment below)
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -213,6 +242,166 @@ impl VariantArray {
}
}
+/// One shredded field of a partially or prefectly shredded variant. For example, suppose the
+/// shredding schema for variant `v` treats it as an object with a single field `a`, where `a` is
+/// itself a struct with the single field `b` of type INT. Then the physical layout of the column
+/// is:
+///
+/// ```text
+/// v: VARIANT {
+/// metadata: BINARY,
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
Review Comment:
```suggestion
/// b: SHREDDED_VARIANT_FIELD {
```
##########
parquet-variant-compute/src/variant_get/output/row_builder.rs:
##########
@@ -0,0 +1,211 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::ArrayRef;
+use arrow::datatypes;
+use arrow::datatypes::ArrowPrimitiveType;
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ return match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ };
+ }
+
+ // Non-empty paths: field access functionality
+ // Helper macro to reduce duplication when wrapping builders with path functionality
+ macro_rules! wrap_with_path {
Review Comment:
Rescuing https://github.com/apache/arrow-rs/pull/8166#discussion_r2286407569 from github oblivion:
If the goal is to avoid duplication, can we move the branch inside the macro, instead of duplicating the entire match statement?
We're not manipulating individual rows here, so the branching should have no measurable impact on performance -- if constructing your builders is the bottleneck, something else is very wrong.
##########
parquet-variant-compute/src/variant_get/output/row_builder.rs:
##########
@@ -0,0 +1,211 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::ArrayRef;
+use arrow::datatypes;
+use arrow::datatypes::ArrowPrimitiveType;
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ return match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ };
+ }
+
+ // Non-empty paths: field access functionality
+ // Helper macro to reduce duplication when wrapping builders with path functionality
+ macro_rules! wrap_with_path {
+ ($inner_builder:expr) => {
+ Ok(Box::new(VariantPathRowBuilder {
+ builder: $inner_builder,
+ path,
+ }) as Box<dyn VariantShreddingRowBuilder + 'a>)
Review Comment:
Is the cast actually needed? The function's return value _should_ constrain it already?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -166,28 +195,28 @@ impl VariantArray {
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {
- ShreddingState::Unshredded { metadata, value } => {
- Variant::new(metadata.value(index), value.value(index))
+ ShreddingState::Unshredded { value, .. } => {
+ // Unshredded case
+ Variant::new(self.metadata.value(index), value.value(index))
}
ShreddingState::Typed { typed_value, .. } => {
+ // Typed case (formerly PerfectlyShredded)
Review Comment:
Ah! I think I figured out the confusion. The spec says this:
> Both `value` and `typed_value` are optional fields used together to encode a single value.
> Values in the two fields must be interpreted according to the following table:
>
> | `value` | `typed_value` | Meaning |
> |----------|---------------|-------------------------------------------------------------|
> | null | null | The value is missing; only valid for shredded object fields |
> | non-null | null | The value is present and may be any type, including null |
> | null | non-null | The value is present and is the shredded type |
> | non-null | non-null | The value is present and is a partially shredded object |
>
> An object is _partially shredded_ when the `value` is an object and the `typed_value` is a shredded object.
Writers must not produce data where both `value` and `typed_value` are non-null, unless the Variant value is an object.
... but those are row-wise concepts -- assuming that both columns are physically present -- and this code is dealing with column-wise concepts where one or both columns could be physically missing. If we produced _that_ table, it might look something like this:
> The `value` and `typed_value` columns are optional columns that together encode columns of variant values. Either or both columns may be physically missing, which can be interpreted according to the following table:
>
> | `value` | `typed_value` | Meaning |
> |----------|---------------|-------------------------------------------------------------|
> | missing | missing | All values are missing; only valid for shredded object fields |
> | present | missing | All values are unshredded, and can be any type, including null |
> | missing | present | All values are present and the shredded type |
> | present | present | At lease some values are present and can be any type, including null |
>
> A shredded object field is perfectly shredded when the `typed_value` column is present and the `value` column is either all-null or missing.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -213,6 +242,166 @@ impl VariantArray {
}
}
+/// One shredded field of a partially or prefectly shredded variant. For example, suppose the
+/// shredding schema for variant `v` treats it as an object with a single field `a`, where `a` is
+/// itself a struct with the single field `b` of type INT. Then the physical layout of the column
+/// is:
+///
+/// ```text
+/// v: VARIANT {
+/// metadata: BINARY,
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
+/// value: BINARY,
+/// typed_value: STRUCT {
+/// a: SHREDDED_VARIANT_FIELD {
+/// value: BINARY,
+/// typed_value: INT,
+/// },
+/// },
+/// },
+/// },
+/// }
+/// ```
+///
+/// In the above, each row of `v.value` is either a variant value (shredding failed, `v` was not an
+/// object at all) or a variant object (partial shredding, `v` was an object but included unexpected
+/// fields other than `a`), or is NULL (perfect shredding, `v` was an object containing only the
+/// single expected field `a`).
+///
+/// A similar story unfolds for each `v.typed_value.a.value` -- a variant value if shredding failed
+/// (`v:a` was not an object at all), or a variant object (`v:a` was an object with unexpected
+/// additional fields), or NULL (`v:a` was an object containing only the single expected field `b`).
+///
+/// Finally, `v.typed_value.a.typed_value.b.value` is either NULL (`v:a.b` was an integer) or else a
+/// variant value (which could be `Variant::Null`).
+#[derive(Debug)]
+pub struct ShreddedVariantFieldArray {
+ /// Reference to the underlying StructArray
+ inner: StructArray,
+ shredding_state: ShreddingState,
+}
+
+#[allow(unused)]
+impl ShreddedVariantFieldArray {
+ /// Creates a new `ShreddedVariantFieldArray` from a [`StructArray`].
+ ///
+ /// # Arguments
+ /// - `inner` - The underlying [`StructArray`] that contains the variant data.
+ ///
+ /// # Returns
+ /// - A new instance of `ShreddedVariantFieldArray`.
+ ///
+ /// # Errors:
+ /// - If the `StructArray` does not contain the required fields
+ ///
+ /// # Requirements of the `StructArray`
+ ///
+ /// 1. An optional field named `value` that is binary, large_binary, or
+ /// binary_view
+ ///
+ /// 2. An optional field named `typed_value` which can be any primitive type
+ /// or be a list, large_list, list_view or struct
+ ///
+ /// Currently, only `value` columns of type [`BinaryViewArray`] are supported.
+ pub fn try_new(inner: ArrayRef) -> Result<Self, ArrowError> {
+ let Some(inner_struct) = inner.as_struct_opt() else {
+ return Err(ArrowError::InvalidArgumentError(
+ "Invalid ShreddedVariantFieldArray: requires StructArray as input".to_string(),
+ ));
+ };
+
+ // Extract value and typed_value fields (metadata is not expected in ShreddedVariantFieldArray)
+ let value = inner_struct.column_by_name("value").and_then(|col| col.as_binary_view_opt().cloned());
+ let typed_value = inner_struct.column_by_name("typed_value").cloned();
+
+ // Use a dummy metadata for the constructor (ShreddedVariantFieldArray doesn't have metadata)
+ let dummy_metadata = arrow::array::BinaryViewArray::new_null(inner_struct.len());
+
+ // Note this clone is cheap, it just bumps the ref count
+ let inner = inner_struct.clone();
+ Ok(Self {
+ inner: inner.clone(),
+ shredding_state: ShreddingState::try_new(dummy_metadata, value, typed_value)?,
+ })
+ }
+
+ /// Return the shredding state of this `VariantArray`
+ pub fn shredding_state(&self) -> &ShreddingState {
+ &self.shredding_state
+ }
+
+ /// Return a reference to the value field of the `StructArray`
+ pub fn value_field(&self) -> Option<&BinaryViewArray> {
+ self.shredding_state.value_field()
+ }
+
+ /// Return a reference to the typed_value field of the `StructArray`, if present
+ pub fn typed_value_field(&self) -> Option<&ArrayRef> {
+ self.shredding_state.typed_value_field()
+ }
+
+ /// Returns a reference to the underlying [`StructArray`].
+ pub fn inner(&self) -> &StructArray {
+ &self.inner
+ }
+}
+
+impl Array for ShreddedVariantFieldArray {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn to_data(&self) -> ArrayData {
+ self.inner.to_data()
+ }
+
+ fn into_data(self) -> ArrayData {
+ self.inner.into_data()
+ }
+
+ fn data_type(&self) -> &DataType {
+ self.inner.data_type()
+ }
+
+ fn slice(&self, offset: usize, length: usize) -> ArrayRef {
+ let inner = self.inner.slice(offset, length);
+ let shredding_state = self.shredding_state.slice(offset, length);
+ Arc::new(Self {
+ inner,
+ shredding_state,
+ })
+ }
+
+ fn len(&self) -> usize {
+ self.inner.len()
+ }
+
+ fn is_empty(&self) -> bool {
+ self.inner.is_empty()
+ }
+
+ fn offset(&self) -> usize {
+ self.inner.offset()
+ }
+
+ fn nulls(&self) -> Option<&NullBuffer> {
+ // According to the shredding spec, ShreddedVariantFieldArray should be
+ // physically non-nullable - SQL NULL is inferred by both value and
+ // typed_value being physically NULL
+ None
+ }
Review Comment:
Rescuing from github oblivion:
* https://github.com/apache/arrow-rs/pull/8166#discussion_r2286276624
> We may need to override the [logical_nulls](https://docs.rs/arrow/latest/arrow/array/trait.Array.html#method.logical_nulls) method.
If `nulls` is hard-wired to return `None` then we _definitely_ need to provide `logical_nulls`?
Otherwise people have to manually dig into the guts of the array?
##########
parquet-variant-compute/src/variant_get/mod.rs:
##########
@@ -500,4 +668,1013 @@ mod test {
VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"),
)
}
+ /// This test manually constructs a shredded variant array representing objects
+ /// like {"x": 1, "y": "foo"} and {"x": 42} and tests extracting the "x" field
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_object_field_access() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from("x"));
+ let result = variant_get(&array, options).unwrap();
+
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect x=1
+ assert_eq!(result_variant.value(0), Variant::Int32(1));
+ // Row 1: expect x=42
+ assert_eq!(result_variant.value(1), Variant::Int32(42));
+ }
+
+ /// Test extracting shredded object field with type conversion
+ #[test]
+ fn test_shredded_object_field_as_int32() {
+ let array = shredded_object_with_x_field_variant_array();
+
+ // Test: Extract the "x" field as Int32Array (type conversion)
+ let field = Field::new("x", DataType::Int32, false);
+ let options = GetOptions::new_with_path(VariantPath::from("x"))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get Int32Array
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
+ assert_eq!(&result, &expected);
+ }
+
+ /// Helper function to create a shredded variant array representing objects
+ ///
+ /// This creates an array that represents:
+ /// Row 0: {"x": 1, "y": "foo"} (x is shredded, y is in value field)
+ /// Row 1: {"x": 42} (x is shredded, perfect shredding)
+ ///
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains object metadata
+ /// - typed_value: StructArray with field "x" (ShreddedVariantFieldArray)
+ /// - value: contains fallback for unshredded fields like {"y": "foo"}
+ /// - The "x" field has typed_value=Int32Array and value=NULL (perfect shredding)
+ fn shredded_object_with_x_field_variant_array() -> ArrayRef {
+ // Create the base metadata for objects
+ let (metadata, y_field_value) = {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut obj = builder.new_object();
+ obj.insert("x", Variant::Int32(42));
+ obj.insert("y", Variant::from("foo"));
Review Comment:
It's illegal for a partially shredded object to mention the same field in both the `value` and `typed_value` columns. To add "x" to the dictionary, manually just use the `VariantBuilder::with_field_names` method?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -166,28 +195,28 @@ impl VariantArray {
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {
- ShreddingState::Unshredded { metadata, value } => {
- Variant::new(metadata.value(index), value.value(index))
+ ShreddingState::Unshredded { value, .. } => {
+ // Unshredded case
+ Variant::new(self.metadata.value(index), value.value(index))
}
ShreddingState::Typed { typed_value, .. } => {
+ // Typed case (formerly PerfectlyShredded)
if typed_value.is_null(index) {
Variant::Null
} else {
typed_value_to_variant(typed_value, index)
}
}
- ShreddingState::PartiallyShredded {
- metadata,
- value,
- typed_value,
- } => {
+ ShreddingState::PartiallyShredded { value, typed_value, .. } => {
+ // PartiallyShredded case (formerly ImperfectlyShredded)
Review Comment:
I realize this PR didn't make the naming decisions, but the wording in the PR is not consistent with these namings. I would personally prefer to fix the naming here to be consistent with how the PR uses it, but we could also try to adjust the PR wording to match these enum variant names.
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3201885089
Thank you @carpecodeum -- I will try and review this tomorrow first thing
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "carpecodeum (via GitHub)" <gi...@apache.org>.
carpecodeum commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3201873362
cc - @alamb @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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#issuecomment-3216689561
Just as a heads up I will be out this upcoming week, so I will be much slower on reviews / merging
Please just let me know if anything needs my attention and I'll get to it as quickyl as I can
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2313747249
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -102,31 +105,57 @@ impl VariantArray {
)));
};
- // Find the value field, if present
- let value = inner
- .column_by_name("value")
- .map(|v| {
- v.as_binary_view_opt().ok_or_else(|| {
- ArrowError::NotYetImplemented(format!(
- "VariantArray 'value' field must be BinaryView, got {}",
- v.data_type()
- ))
- })
- })
- .transpose()?;
-
- // Find the typed_value field, if present
- let typed_value = inner.column_by_name("typed_value");
+ // Extract value and typed_value fields
+ let value = if let Some(value_col) = inner.column_by_name("value") {
+ if let Some(binary_view) = value_col.as_binary_view_opt() {
+ Some(binary_view.clone())
+ } else {
+ return Err(ArrowError::NotYetImplemented(format!(
+ "VariantArray 'value' field must be BinaryView, got {}",
+ value_col.data_type()
+ )));
+ }
+ } else {
+ None
+ };
+ let typed_value = inner.column_by_name("typed_value").cloned();
// Note these clones are cheap, they just bump the ref count
- let inner = inner.clone();
- let shredding_state =
- ShreddingState::try_new(metadata.clone(), value.cloned(), typed_value.cloned())?;
-
Ok(Self {
+ inner: inner.clone(),
+ metadata: metadata.clone(),
+ shredding_state: ShreddingState::try_new(metadata.clone(), value, typed_value)?,
+ })
+ }
+
+ #[allow(unused)]
+ pub(crate) fn from_parts(
+ metadata: BinaryViewArray,
+ value: Option<BinaryViewArray>,
+ typed_value: Option<ArrayRef>,
+ nulls: Option<NullBuffer>,
+ ) -> Self {
+ let mut builder =
+ StructArrayBuilder::new().with_field("metadata", Arc::new(metadata.clone()));
+ if let Some(value) = value.clone() {
+ builder = builder.with_field("value", Arc::new(value));
+ }
+ if let Some(typed_value) = typed_value.clone() {
+ builder = builder.with_field("typed_value", typed_value);
+ }
+ if let Some(nulls) = nulls {
Review Comment:
Those other two are present in both `inner` and `shredding_state`, hence the`clone` calls. The `nulls` are only used once, so no `clone` needed.
--
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] Support Shredded Objects in variant_get [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8166:
URL: https://github.com/apache/arrow-rs/pull/8166#discussion_r2313762359
##########
parquet-variant-compute/src/variant_get/output/row_builder.rs:
##########
@@ -0,0 +1,211 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::ArrayRef;
+use arrow::datatypes;
+use arrow::datatypes::ArrowPrimitiveType;
+use arrow::error::{ArrowError, Result};
+use parquet_variant::{Variant, VariantPath};
+
+use crate::VariantArrayBuilder;
+
+use std::sync::Arc;
+
+pub(crate) fn make_shredding_row_builder<'a>(
+ //metadata: &BinaryViewArray,
+ path: VariantPath<'a>,
+ data_type: Option<&'a datatypes::DataType>,
+) -> Result<Box<dyn VariantShreddingRowBuilder + 'a>> {
+ use arrow::array::PrimitiveBuilder;
+ use datatypes::Int32Type;
+
+ // support non-empty paths (field access) and some empty path cases
+ if path.is_empty() {
+ return match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Return PrimitiveInt32Builder for type conversion
+ let builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ Ok(Box::new(builder))
+ }
+ None => {
+ // Return VariantArrayBuilder for VariantArray output
+ let builder = VariantArrayShreddingRowBuilder::new(16);
+ Ok(Box::new(builder))
+ }
+ _ => {
+ // only Int32 supported for empty paths
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with empty path and data_type={:?} not yet implemented",
+ data_type
+ )))
+ }
+ };
+ }
+
+ // Non-empty paths: field access functionality
+ // Helper macro to reduce duplication when wrapping builders with path functionality
+ macro_rules! wrap_with_path {
+ ($inner_builder:expr) => {
+ Ok(Box::new(VariantPathRowBuilder {
+ builder: $inner_builder,
+ path,
+ }) as Box<dyn VariantShreddingRowBuilder + 'a>)
+ };
+ }
+
+ match data_type {
+ Some(datatypes::DataType::Int32) => {
+ // Create a primitive builder and wrap it with path functionality
+ let inner_builder = PrimitiveVariantShreddingRowBuilder {
+ builder: PrimitiveBuilder::<Int32Type>::new(),
+ };
+ wrap_with_path!(inner_builder)
+ }
+ None => {
+ // Create a variant array builder and wrap it with path functionality
+ let inner_builder = VariantArrayShreddingRowBuilder::new(16);
+ wrap_with_path!(inner_builder)
+ }
+ _ => {
+ // only Int32 and VariantArray supported
+ Err(ArrowError::NotYetImplemented(format!(
+ "variant_get with path={:?} and data_type={:?} not yet implemented",
+ path, data_type
+ )))
+ }
+ }
+}
+
+/// Builder for shredding variant values into strongly typed Arrow arrays.
+///
+/// Useful for variant_get kernels that need to extract specific paths from variant values, possibly
+/// with casting of leaf values to specific types.
+pub(crate) trait VariantShreddingRowBuilder {
+ fn append_null(&mut self) -> Result<()>;
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool>;
+
+ fn finish(&mut self) -> Result<ArrayRef>;
+}
+
+/// A thin wrapper whose only job is to extract a specific path from a variant value and pass the
+/// result to a nested builder.
+struct VariantPathRowBuilder<'a, T: VariantShreddingRowBuilder> {
Review Comment:
Not sure I understand? The caller already followed shredded path steps as far as possible before creating _any_ builder. This path builder is used to extract the remaining path steps, on a row-by-row basis, from the values of a binary variant column the caller encountered before the path was exhausted. Not sure how that could be moved to the caller?
--
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