Skip to content

[Variant] Add variant_get and Shredded VariantArray #8021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 18 commits into from
Aug 7, 2025
Merged

[Variant] Add variant_get and Shredded VariantArray #8021

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
3ba9fe0
[Variant] Strawman / infrastructure for variant_get of shredded values
alamb Jul 18, 2025
f458ec5
Apply suggestions from code review
alamb Aug 5, 2025
52cf52b
Merge remote-tracking branch 'apache/main' into alamb/variant_shreddi…
alamb Aug 5, 2025
6b66650
remove completed todo comment
alamb Aug 5, 2025
924155c
Move validation logic to `ShreddingState::try_new`
alamb Aug 5, 2025
5cb1068
Apply suggestions from code review
alamb Aug 6, 2025
9c22f85
cleanup
alamb Aug 6, 2025
eee199e
Avoid panics in production code
alamb Aug 6, 2025
264356b
Merge remote-tracking branch 'apache/main' into alamb/variant_shreddi…
alamb Aug 6, 2025
336a0a3
comments
alamb Aug 6, 2025
016cee1
Merge remote-tracking branch 'apache/main' into alamb/variant_shreddi…
alamb Aug 7, 2025
3e8820b
Fix up merge
alamb Aug 7, 2025
55a7ae6
Update parquet-variant-compute/src/variant_array.rs
alamb Aug 7, 2025
85e7194
Merge branch 'alamb/variant_shredding_strawman' of github.com:alamb/a…
alamb Aug 7, 2025
6e38490
fix compilation
alamb Aug 7, 2025
0ccd0f8
update docs
alamb Aug 7, 2025
f8b2df4
Rename ShreddingState to conform better to spec
alamb Aug 7, 2025
2355247
Update name in output builder
alamb Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Update name in output builder
  • Loading branch information
@alamb
alamb committed Aug 7, 2025
commit 2355247e0f20f1d7fcb71bfa6f7c43d70a7d6e7d
2 changes: 1 addition & 1 deletion parquet-variant-compute/src/variant_get/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub fn variant_get(input: &ArrayRef, options: GetOptions) -> Result<ArrayRef> {
ShreddingState::Typed {
metadata,
typed_value,
} => output_builder.fully_shredded(variant_array, metadata, typed_value),
} => output_builder.typed(variant_array, metadata, typed_value),
ShreddingState::Unshredded { metadata, value } => {
output_builder.unshredded(variant_array, metadata, value)
}
Expand Down
2 changes: 1 addition & 1 deletion parquet-variant-compute/src/variant_get/output/mod.rs
View file
Open in desktop
Copy link
Contributor

@scovich scovich Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the output builder seems to be fully column-oriented -- it assumes that all values for each leaf column are extracted in a tight loop. This can work for primitive builders, but nested builders will quickly run into pathing and efficiency problems.

I think we'll need to do something similar to the JSON builder, with a row-oriented approach where each level of a nested builder receives an already-constructed Variant for the current row and does a field extract for each child builder; child builders can then cast the result directly or recurse further as needed (based on their own type). And then the top-level builder call would construct a Variant for each row to kick-start the process.

But see the other comment -- to the extend that the shredding aligns nicely, we can hoist a subset of this per-row pathing of the append method up to columnar pathing of the builder's constructor and finalizer.

Copy link
Contributor Author

@alamb alamb Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this aligns with my thinking and is nicely described -- I will use it as the base for the next set of tickets perhaps

Copy link
Contributor Author

@alamb alamb Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed

Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub(crate) trait OutputBuilder {
) -> Result<ArrayRef>;

/// output for a perfectly shredded variant array
fn fully_shredded(
fn typed(
&self,
variant_array: &VariantArray,
metadata: &BinaryViewArray,
Expand Down
2 changes: 1 addition & 1 deletion parquet-variant-compute/src/variant_get/output/primitive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ impl<'a, T: ArrowPrimitiveVariant> OutputBuilder for PrimitiveOutputBuilder<'a,
Ok(Arc::new(array))
}

fn fully_shredded(
fn typed(
&self,
_variant_array: &VariantArray,
_metadata: &BinaryViewArray,
Expand Down
2 changes: 1 addition & 1 deletion parquet-variant-compute/src/variant_get/output/variant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ impl<'a> OutputBuilder for VariantOutputBuilder<'a> {
Ok(Arc::new(array_builder.build()))
}

fn fully_shredded(
fn typed(
&self,
variant_array: &VariantArray,
// TODO(perf): can reuse the metadata field here to avoid re-creating it
Expand Down
Loading