Skip to content

Conversation

@liamzwbao
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

  • Move ArrayVariantToArrowRowBuilder from shred_variant to variant_to_arrow so it can be shared with variant_get.
  • Reorder definitions in variant_to_arrow to clarify the hierarchy: start with the top-level VariantToArrowRowBuilder, then second-level builders such as PrimitiveVariantToArrowRowBuilder and ArrayVariantToArrowRowBuilder, etc.
  • Add tests for variant_get with lists.

Are these changes tested?

Yes

Are there any user-facing changes?

variant_get now supports lists.

@github-actions github-actions bot added the parquet-variant parquet-variant* crates label Dec 27, 2025
@liamzwbao liamzwbao marked this pull request as ready for review December 27, 2025 08:15
Copy link
Contributor

@scovich scovich left a comment

Choose a reason for hiding this comment

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

I only had a few minutes to skim, and I couldn't tell quickly what was code movement vs. actual changes. Would there be a way to split them out into separate commits or something? Or at least describe clearly what moved vs. what changed?

The second commit in the stack was helpful that way, but the first commit still seemed to do an awful lot.

@alamb
Copy link
Contributor

alamb commented Dec 31, 2025

I only had a few minutes to skim, and I couldn't tell quickly what was code movement vs. actual changes. Would there be a way to split them out into separate commits or something? Or at least describe clearly what moved vs. what changed?

The second commit in the stack was helpful that way, but the first commit still seemed to do an awful lot.

I am also happy to help review / quickly merge a PR that is just code movement

@liamzwbao liamzwbao force-pushed the issue-8082-variant-get-list branch from 136640a to 649cfcf Compare January 5, 2026 22:14
Copy link
Member

@klion26 klion26 left a comment

Choose a reason for hiding this comment

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

LGTM, Maybe we can create an issue to track the VariantPathElement::Index case.

}

#[test]
fn test_variant_get_list_like_safe_cast() {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add some cases for that with VariantPathElement::Field(We don't support VariantPathElement::Index in the current pr) or some perfectly shredded case

When trying to use shred_variant to generate some perfectly shredded cases, I found that the member variable value is not null for the return value of shred_variant. Is this the expected behavior? According to the parquet-vairant-spec

If the value is not an array, typed_value must be null. If the value is an array, value must be null.

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parquet-variant parquet-variant* crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Variant] Support Shredded Lists/Array in variant_get

4 participants