-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-13397: [R] Update arrow.Rmd vignette #11202
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
Conversation
jonkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one change / question about the link there
r/vignettes/arrow.Rmd
Outdated
|
|
||
| ^2^: Some Arrow data types do not have an R equivalent and will raise an error | ||
| if cast to or mapped to via a schema. See | ||
| [this discussion section](#no-compat-type) for an example of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "See [this discussion section]" could we use something like "See [the XXX section]" (the this to me seems a little bit odd.
Also, where is this supposed to be linking to? I've tried to find the section but haven't yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, I'm an idiot, I pasted this back from the cookbook without reading properly, will update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed now.
jonkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. One small nit and one question out of curiosity
|
|
||
| ^4^: Due to the limitation of R `factor`s, Arrow `dictionary` values are coerced to string when translated to R if they are not already strings. | ||
| ^4^: Due to the limitation of R factors, Arrow `dictionary` values are coerced | ||
| to string when translated to R if they are not already strings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't relevant in this doc, but now I'm curious: what is the limitation of R factors here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a clue tbh; I think @ianmcook may(?) have written this text originally, so might have a better idea than me
| yields a `bit64::integer64` vector) by setting `options(arrow.int64_downcast = FALSE)`. | ||
|
|
||
| ^2^: Some Arrow data types do not have an R equivalent and will raise an error | ||
| if cast to or mapped to via a schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might we mention something like "do not yet have an R equivalent"? At least for duration I imagine we will map it on to lubridate's duration type at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Update the table in arrow.Rmd to add extra explanations and re-order some of the contents. Closes apache#11202 from thisisnic/ARROW-13397_arrow.Rmd Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Jonathan Keane <jkeane@gmail.com>
Update the table in arrow.Rmd to add extra explanations and re-order some of the contents.