Skip to content

Conversation

@scovich
Copy link
Contributor

@scovich scovich commented Sep 13, 2025

Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.

  • Closes #NNN.

Rationale for this change

variant_get has code to support reading a struct from perfectly shredded variant data, but no test coverage.

What changes are included in this PR?

Add the missing coverage.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the parquet-variant parquet-variant* crates label Sep 13, 2025
@scovich
Copy link
Contributor Author

scovich commented Sep 13, 2025

CC @alamb @codephage2020 -- another quickie (once the PR it depends on merges)

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

Copy link
Member

Choose a reason for hiding this comment

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

Does the comment dummy value for row 2 here mean that row 2 is inner field null?

Copy link
Member

Choose a reason for hiding this comment

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

Does this(row 3) mean writer did not respect the variant spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on null rows, the array can contain any physically valid value; it will anyway be ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and they have to contain something to keep row counts in sync)

@scovich scovich force-pushed the variant-get-struct-tests branch from bf83990 to 7328d19 Compare September 15, 2025 13:52
Copy link
Contributor

@codephage2020 codephage2020 left a comment

Choose a reason for hiding this comment

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

Looks good to me ! Thank you !

@scovich
Copy link
Contributor Author

scovich commented Sep 16, 2025

@mbrobbel -- any reason not to merge this? (I don't have rights)

@mbrobbel mbrobbel merged commit 71eede6 into apache:main Sep 16, 2025
12 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 16, 2025

woohoo we are cooking the last few days in arrow-rs

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.

5 participants