[Variant] Adding code to store metadata and value references in VariantArray #7945

abacef · 2025-07-16T21:41:37Z

Which issue does this PR close?

Closes [Variant] Improve VariantArray performance by storing the index of the metadata and value arrays #7920.

Are these changes tested?

Tests were already implemented

Are there any user-facing changes?

None

codephage2020

Hi, @abacef. I just worked on this too. I left a few suggestions that might help improve this PR. Feel free to take what works for you! 😊

codephage2020 · 2025-07-17T03:26:45Z

parquet-variant-compute/src/variant_array.rs

@@ -88,7 +90,10 @@ impl VariantArray {
            ));
        };
        // Ensure the StructArray has a metadata field of BinaryView
-        let Some(metadata_field) = inner.fields().iter().find(|f| f.name() == "metadata") else {
+
+        let cloned = inner.clone();


Maybe we can do the cloning at the end.

I dont know how it is possible since the metadata_field and value_field need to reference the cloned StructArray

It is believed that the metadata_field and value_field refer to the StructArray rather than the cloned StructArray.

When doing a clone, is it just incrementing the reference count to the function parameter? If so, maybe finding the metadata reference and the value reference on either the passed in StructArray or the clone will come up with the same value?

Yes, that's correct. The final result is the same. However, what I want to say is that we can perform the clone operation after the verification. These verification and value retrieval operations may fail. If we place the clone operation at the end, we can avoid unnecessary operations.

I think it is ok to clone the argument here even if it will be wasteful on the error case, as the error case is not commonly expected

Make sense.

codephage2020 · 2025-07-17T03:36:11Z

parquet-variant-compute/src/variant_array.rs

@@ -169,8 +184,13 @@ impl Array for VariantArray {
    }

    fn slice(&self, offset: usize, length: usize) -> ArrayRef {
+        let slice = self.inner.slice(offset, length);
+        let met = VariantArray::find_metadata_field(&slice).unwrap();
+        let val = VariantArray::find_value_field(&slice).unwrap();


I'm thinking about whether I can directly use the index without searching.

I agree it is correct to slice the child arrays

so something like

let val = self.value_ref.slice(offset, length);

Oh I see, makes sense.

alamb

Thank you @abacef -- I think this looks great.

@codephage2020 has some good ideas and I left some other comments that I think are worth considering, but I don't think any of them are required to merge this PR

alamb · 2025-07-17T15:16:05Z

parquet-variant-compute/src/variant_array.rs

@@ -88,7 +90,10 @@ impl VariantArray {
            ));
        };
        // Ensure the StructArray has a metadata field of BinaryView
-        let Some(metadata_field) = inner.fields().iter().find(|f| f.name() == "metadata") else {
+
+        let cloned = inner.clone();


I think it is ok to clone the argument here even if it will be wasteful on the error case, as the error case is not commonly expected

alamb · 2025-07-17T15:17:51Z

parquet-variant-compute/src/variant_array.rs

@@ -169,8 +184,13 @@ impl Array for VariantArray {
    }

    fn slice(&self, offset: usize, length: usize) -> ArrayRef {
+        let slice = self.inner.slice(offset, length);
+        let met = VariantArray::find_metadata_field(&slice).unwrap();
+        let val = VariantArray::find_value_field(&slice).unwrap();


I agree it is correct to slice the child arrays

so something like

let val = self.value_ref.slice(offset, length);

alamb · 2025-07-17T15:20:07Z

parquet-variant-compute/src/variant_array.rs

@@ -59,6 +59,8 @@ pub struct VariantArray {
    /// Dictionary-Encoded, preferably (but not required) with an index type of
    /// int8.
    inner: StructArray,
+    metadata_ref: ArrayRef,


It would be nice to add some comment that explain this is a cached copy of the metadata / value child arrays

…cing existing columns instead of finding the columns again

codephage2020

LGTM . 🚀

alamb · 2025-07-18T13:00:39Z

Thanks again @abacef and @codephage2020

abacef added 2 commits July 16, 2025 14:38

Adding code to store metadata and value references in VariantArray

a71f39d

ran cargo fmt

e75235a

codephage2020 reviewed Jul 17, 2025

View reviewed changes

alamb changed the title ~~Adding code to store metadata and value references in VariantArray~~ [Variant] Adding code to store metadata and value references in VariantArray Jul 17, 2025

alamb approved these changes Jul 17, 2025

View reviewed changes

abacef added 2 commits July 17, 2025 11:23

clone after handling error cases for try_new, for array slice try sli…

6c759e2

…cing existing columns instead of finding the columns again

Added comments to the new fields in VariantArray

8ab7414

codephage2020 approved these changes Jul 18, 2025

View reviewed changes

alamb merged commit a984ca7 into apache:main Jul 18, 2025
9 checks passed

[Variant] Adding code to store metadata and value references in VariantArray #7945

[Variant] Adding code to store metadata and value references in VariantArray #7945

Uh oh!

Conversation

abacef commented Jul 16, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codephage2020 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codephage2020 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Jul 18, 2025

Uh oh!

Uh oh!

abacef commented Jul 16, 2025 •

edited by alamb

Loading