Skip to content

GH-33390: [R] Field-level metadata#49631

Merged
thisisnic merged 4 commits intoapache:mainfrom
max-romagnoli:GH-33390-r-field-metadata
Apr 10, 2026
Merged

GH-33390: [R] Field-level metadata#49631
thisisnic merged 4 commits intoapache:mainfrom
max-romagnoli:GH-33390-r-field-metadata

Conversation

@max-romagnoli
Copy link
Copy Markdown
Contributor

@max-romagnoli max-romagnoli commented Apr 1, 2026

Rationale for this change

  • field() in R unlike Python does not support field-level metadata.

What changes are included in this PR?

  • New active bindings on Field: $HasMetadata, $metadata
  • New methods on Field: $WithMetadata(), $RemoveMetadata()
  • New check_metadata parameter in Field$Equals(), defaulting to FALSE
  • Tests for the above

Are these changes tested?

  • Yes, unit tests have been added, the functionality was also tested locally in R

Are there any user-facing changes?

  • Yes, metadata= now works without throwing an error.
  • No breaking changes, since this was already included but errored before and parameter order was not changed in the implementation.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 1, 2026

⚠️ GitHub issue #33390 has been automatically assigned in GitHub to PR creator.

Copy link
Copy Markdown
Member

@thisisnic thisisnic left a comment

Choose a reason for hiding this comment

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

Thanks for making this PR! I think it just needs a few more tests:

  • WithMetadata(NULL) - sets empty metadata
  • a roundtrip test to check we can save and load it and it matches
  • (what happens if there are duplicate keys?)
  • nested type, e.g. a field with struct type containing their own metadata

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 6, 2026
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 10, 2026
@max-romagnoli max-romagnoli requested a review from thisisnic April 10, 2026 09:00
@max-romagnoli
Copy link
Copy Markdown
Contributor Author

Thanks for making this PR! I think it just needs a few more tests:

@thisisnic I have added the four tests there. Let me know if thinks look good or more changes are needed. Thanks!

Copy link
Copy Markdown
Member

@thisisnic thisisnic 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, thanks for making this PR!

@thisisnic thisisnic merged commit b738178 into apache:main Apr 10, 2026
15 checks passed
@thisisnic thisisnic removed the awaiting change review Awaiting change review label Apr 10, 2026
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit b738178.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants