Skip to content

[Variant] Add nullable arg to StructArrayBuilder::with_field #8342

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

Jump to bottom
Merged
alamb merged 3 commits into from
Sep 15, 2025
Merged

[Variant] Add nullable arg to StructArrayBuilder::with_field #8342

alamb merged 3 commits into from
Sep 15, 2025

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

StructArrayBuilder::with_field is currently hard-wired to assume the field will be nullable. This is unhelpful when adding non-nullable fields such as variant a metadata column.

What changes are included in this PR?

Add a third parameter, nullable.

Are these changes tested?

The builder is heavily used by unit tests, which have been adjusted to pass the new param.

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 -- should be a quickie

@scovich scovich mentioned this pull request Sep 13, 2025
Merged
alamb
alamb approved these changes Sep 14, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb 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 -- thanks @scovich

@alamb
Copy link
Contributor

alamb commented Sep 14, 2025

It looks like there is a conflict in this PR

@scovich
Copy link
Contributor Author

scovich commented Sep 15, 2025

It looks like there is a conflict in this PR

Some kind of github confusion, methinks? The git cli handled it fine 🤷

arrow-rs % git merge oss/main
Auto-merging parquet-variant-compute/src/variant_get.rs
Merge made by the 'ort' strategy.
 parquet-variant-compute/src/lib.rs                                                     |  1 +
 parquet-variant-compute/src/{variant_get/mod.rs => variant_get.rs}                     | 10 ++++------
 parquet-variant-compute/src/variant_get/output/mod.rs                                  | 18 ------------------
 parquet-variant-compute/src/{variant_get/output/row_builder.rs => variant_to_arrow.rs} | 57 ++++++++++++++++++++++++++++++++-------------------------
 4 files changed, 37 insertions(+), 49 deletions(-)
 rename parquet-variant-compute/src/{variant_get/mod.rs => variant_get.rs} (99%)
 delete mode 100644 parquet-variant-compute/src/variant_get/output/mod.rs
 rename parquet-variant-compute/src/{variant_get/output/row_builder.rs => variant_to_arrow.rs} (73%)

codephage2020
codephage2020 approved these changes Sep 15, 2025
View reviewed changes
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!

@codephage2020 codephage2020 mentioned this pull request Sep 15, 2025
Merged
@alamb
Copy link
Contributor

alamb commented Sep 15, 2025

Merged up to resolve a conflict per @codephage2020

@alamb alamb merged commit bfdc31b into apache:main Sep 15, 2025
12 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 15, 2025

Thanks @scovich and @codephage2020

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

Reviewers

@alamb alamb alamb approved these changes

+1 more reviewer

@codephage2020 codephage2020 codephage2020 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

parquet-variant parquet-variant* crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@scovich @alamb @codephage2020