Skip to content
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.

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

refactor(r): Use JSON in experimental R vctrs extension type #533

Merged
merged 6 commits into from
Jun 20, 2024

Conversation

paleolimbot
Copy link
Member

This PR updates the "vctrs extension type" to use JSON as its metadata serialization. JSON is use for most other extension types and using it here provides some workaround for non-R consumers that encounter this type elsewhere.

The serializer and deserializer use the same format as jsonlite::serializeJSON(), but restrict the types of objects that it is willing to serialize or deserialize.

@amoeba
Copy link
Member

amoeba commented Jun 20, 2024

This looks good @paleolimbot. I am getting one test failure locally though:

══ Results ═══════════════════════════════════════════════════════════════════════════
Duration: 4.6 s

── Failed tests ──────────────────────────────────────────────────────────────────────
Failure (test-extension.R:83:3): as_nanoarrow_array() dispatches on registered extension spec
`as_nanoarrow_array(...)` did not throw the expected error.

[ FAIL 1 | WARN 0 | SKIP 0 | PASS 1426 ]

@paleolimbot
Copy link
Member Author

Thank you for the review!

Is there any chance that the latest commit fixes the issue? (I'm wondering if it's related to the fact that we can't really unregister s3 methods and this may have changed in R 4.4).

@amoeba
Copy link
Member

amoeba commented Jun 20, 2024

Yep, that did it. All tests pass now.

@paleolimbot paleolimbot merged commit 05cdc1b into apache:main Jun 20, 2024
13 checks passed
@paleolimbot paleolimbot deleted the r-vctrs-extension-type branch June 21, 2024 12:11
@paleolimbot paleolimbot added this to the nanoarrow 0.6.0 milestone Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants