Address issue #1105 #1164
Address issue #1105 #1164
Conversation
|
Thanks, but how does it compare to #1154? Is this an addition or a duplicate? |
| @@ -11,8 +11,9 @@ pub enum RArrowArrayClass { | |||
| NanoArrowArray, | |||
| } | |||
|
|
|||
| impl<'a> FromRobj<'a> for RArrowArrayClass { | |||
| fn from_robj(robj: &Robj) -> std::result::Result<Self, &'static str> { | |||
| impl TryFrom<&Robj> for RArrowArrayClass { | |||
There was a problem hiding this comment.
Note that FromRobj was removed in favor of TryFrom
| @@ -153,10 +153,11 @@ fn recursive_robjname2series_tree(x: &Robj, name: &str) -> pl::PolarsResult<Seri | |||
| } | |||
|
|
|||
| Rtype::Raw => { | |||
| let rpolars_raw_list = list!(x) | |||
| let mut binding = list!(x); | |||
There was a problem hiding this comment.
modifying attributes modifies in place. Previously, this didn't require a mutable reference giving false confidence. Now it requires a mutable reference. Additionally, the same type is returned after modifying the attributes. So if you use set_class() or set_attrib() on a List it will return a mut List.
| .set_class(["rpolars_raw_list", "list"]) | ||
| .map_err(|err| pl::polars_err!(ComputeError: err.to_string()))?; | ||
| recursive_robjname2series_tree(&rpolars_raw_list, name) | ||
| recursive_robjname2series_tree(&rpolars_raw_list.clone().into_robj(), name) |
There was a problem hiding this comment.
We increment the reference counter and convert into an Robj for compatibility with this function. I did not want to modify the function itself.
| @@ -49,6 +49,7 @@ pub fn pl_series_to_list( | |||
| .collect_robj() | |||
| .set_class(&["integer64"]) | |||
| .expect("internal error could not set class label 'integer64'") | |||
| .clone() | |||
There was a problem hiding this comment.
Increment reference counter to go from &mut Robj to Robj and likewise below.
| }) | ||
| .map(|mut robj| robj.set_attrib("tu", "ns")) | ||
| .map(|mut robj| robj.set_attrib("tu", "ns").cloned()) |
There was a problem hiding this comment.
Similarly from Result<&mut Robj, E> to Result<Robj, E>
#1154 was the inspiration for extendr/extendr#824. This PR requires fewer LOC changed. I hope these are helpful for you! I left comments in this PR to indicate why the changes were necessary. You can choose which, if either, you want to merge. |
|
I think it is fair to say that you have 2 PRs because each of us were asked independently to take a look at this 😆 |
|
Sorry, I thought you had seen the linked PR in #1105. Since our feedback in #1154 looks important (#1154 (comment)) I guess we'll prioritize it over this one, but the comments here are much appreciated! |
This PR bumps the version of extendr used to address R-devel changes. It also requires a specific revision that was merged in extendr/extendr#824.
CC @etiennebacher