GH-33390: [R] Field-level metadata#49631
Conversation
|
|
thisisnic
left a comment
There was a problem hiding this comment.
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
@thisisnic I have added the four tests there. Let me know if thinks look good or more changes are needed. Thanks! |
thisisnic
left a comment
There was a problem hiding this comment.
Looks good, thanks for making this PR!
|
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. |
### 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 Issue: apache#33390 Lead-authored-by: Max Romagnoli <massimiliano.romagnoli02@gmail.com> Co-authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>
Rationale for this change
field()in R unlike Python does not support field-level metadata.What changes are included in this PR?
Field:$HasMetadata,$metadata$WithMetadata(),$RemoveMetadata()check_metadataparameter inField$Equals(), defaulting to FALSEAre these changes tested?
Are there any user-facing changes?