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?
Field:$HasMetadata,$metadata$WithMetadata(),$RemoveMetadata()check_metadataparameter inField$Equals(), defaulting to FALSEAre these changes tested?
Are there any user-facing changes?