-
Notifications
You must be signed in to change notification settings - Fork 975
[VARIANT] Path-based Field Extraction for VariantArray #7946
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.
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
base: main
Are you sure you want to change the base?
Conversation
18d88b0
to
b1afed1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR @carpecodeum
This is very cool
I think there is already a variant_get
implementation in https://github.com/apache/arrow-rs/blob/d809f19bc0fe2c3c1968f5111b6afa785d2e8bcd/parquet-variant-compute/src/variant_get.rs#L35-L34 contributed by @Samyak2
To take the next steps and implement shredding I think we will need two things:
- A way to create shredded variants
- A way to represent shredded variants
The idea of removing fields from Variant
s is interesting, though I wonder if that is an operation we would ever want to do on single Variant
instance -- it seems like removing fields for shredding will require copying the underlying bytes anyways, so I was thinking we might just want to create an output variant array entirely
Something like
fn variant_shred(input: VariantArray, output: VariantArray, schema: SchemaRef)
Maybe it is worth looking at how the java or go implementations work
@@ -0,0 +1,2 @@ | |||
[build] | |||
rustflags = ["-A", "unknown-lints", "-A", "clippy::transmute-int-to-float"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required?
{ | ||
let mut variant_builder = VariantBuilder::new(); | ||
{ | ||
let mut obj = variant_builder.new_object(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note I think these tests wil lbe easier to write after the API in
use parquet_variant::VariantMetadata; | ||
use std::collections::HashSet; | ||
|
||
/// Represents a path element in a variant path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think if you merge up from main this code will no longer be required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree, a lot of this becomes redundant but get_field_bytes
is able to get field bytes from an object at the byte level, will this wont be required too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite sure what you are asking.
I think it might help to move shredded variant forward by writing the tests / examples of how variant_get should work with shredded arrays
I tried to work up a simple example here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw your PR it looks great, im myself trying to work up a few examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea would to be to try and create the other examples from https://docs.google.com/document/d/1pw0AWoMQY3SjD7R4LgbPvMjG_xSCtXp3rZHkVp9jpZ4/ in code
9a616b5
to
c712747
Compare
/// let path = VariantPath::field("name"); | ||
/// let name_variant = variant_array.get_path(0, &path); | ||
/// ``` | ||
pub fn get_path(&self, index: usize, path: &VariantPath) -> Option<Variant> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how this is different than variant_get
🤔
pub fn variant_get(input: &ArrayRef, options: GetOptions) -> Result<ArrayRef> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its quite similar, I started working on it, and didn't realise that the idea is very similar to PR 7919 until Wednesday. I'm working on making a lot of changes here and removing the redundancy between my PR and the variant_get functionality, which might include the tests and examples in this PR itself, also will try to do the optimisations for variant_get
Is there any issue for implementing this? I would love to work on it |
I think we are discussing reading shredded variants on We are discussing creating shredded variants on I don't think we have enough of an idea of how this will work to break them down into finer grained tasks yet. |
Which issue does this PR close?
This PR implements efficient path-based field extraction and manipulation capabilities for
VariantArray
, enabling direct access to nested fields without expensive unshredding operations.Follow-up on #7919
variant_get
kernel for shredded variants #7941Rationale for this change
This work builds directly on the path navigation concepts introduced in #7919, sharing the fundamental VariantPathElement design with Field and Index variants. While PR #7919 provided a compute kernel approach with a variant_get function, this PR provides instance-based methods directly on VariantArray with a builder API using owned strings rather than PR #7919 vector-based approach.
This is a draft still, as the changes for #7919 got merged today, I still have to incorporate those changes, and looking forward to reviews and suggestions.
This PR is complementary to #7921, which implements schema-driven shredding during array construction. This PR provides runtime path-based access to both shredded and unshredded data, creating a complete solution for both efficient construction and efficient access of variant data.
Big Thanks to @mprammer @PinkCrow007 for their continued support throughout my
Variant
explorationWhat changes are included in this PR?
Field removal operations through methods like
remove_field
andremove_fields
enable removal of specific fields from variant data, crucial for shredding operations where temporary or debug fields need to be stripped.field_operations.rs
provides direct binary manipulation through functions likeget_path_bytes
,extract_field_bytes
, andremove_field_bytes
that operate on raw binary format without constructing intermediate objects.variant_parser.rs
supports all variant types with parsers for 17 different primitive types, providing the foundation for efficient binary navigation.The performance-critical byte operations could serve as the underlying implementation for PR #7919's compute kernel, potentially providing better performance for batch operations by avoiding object construction overhead. The field removal capabilities could extend PR #7919's functionality beyond extraction to comprehensive field manipulation. The instance-based approach provides different ergonomics that complement PR #7919's compute kernel approach.
This PR focuses on runtime access and manipulation rather than construction-time optimization, leaving build-time schema-driven shredding to PR #7921. Future work is integration with PR #7919's compute kernel approach, potentially using this PR's byte-level operations as the underlying implementation.
Are these changes tested?
Yes, tests are added
Are there any user-facing changes?
Not yet