feat: Expressify list.shift #11320
feat: Expressify list.shift #11320
Conversation
github-actions
bot
added
enhancement
| @@ -188,6 +188,47 @@ impl ListChunked { | |||
| unsafe { self.amortized_iter().for_each(f) } | |||
| } | |||
|
|
|||
| /// Zip with a `ChunkedArray` then apply a binary function `F` elementwise. | |||
| #[must_use] | |||
| pub fn zip_and_apply_amortized<'a, T, I, F>(&'a self, ca: &'a ChunkedArray<T>, mut f: F) -> Self | |||
There was a problem hiding this comment.
I don't know what name would be more appropriate, but I assume that having a safe version would be good.
reswqa
requested review from
ritchie46,
stinodego,
alexander-beedie and
orlp
as code owners
| _ => out, | ||
| } | ||
| }) | ||
| .collect_trusted() |
There was a problem hiding this comment.
I think we should make this a bit more resilient to ambiguous dtypes. We can use collect_ca_trusted_with_dtype and accept the list inner type as argument of this function and then construct the DataType::List<inner> with that.
In a later PR we can do the same with apply_amortized.
There was a problem hiding this comment.
pub fn zip_and_apply_amortized<'a, T, I, F>(&'a self, ca: &'a ChunkedArray<T>, mut f: F) -> SelfThis function will return Self, maybe we don't need accept an inner type as argument, but pass self.dtype().clone() to collect_ca_trusted_with_dtype directly? What do you think?
In a later PR we can do the same with apply_amortized.
I tend do this for apply_amortized also in this PR, after that, I think we can remove the call to Ok(self.same_type(out)) in lst_shift safely.
There was a problem hiding this comment.
This function will return Self, maybe we don't need accept an inner type as argument, but pass self.dtype().clone() to collect_ca_trusted_with_dtype directly? What do you think?
Yes, that would be sufficient.
There was a problem hiding this comment.
A bad news: It seems that our implementation of collect_ca_trusted_with_dtype will forward to collect_arr_trusted_with_dtype and just wrap this single Array to ChunkedArray.
So it requires ArrayFromIterDtype of Option<Series> or Series for ListArray(Because the closure of apply_amortized is F: FnMut(UnstableSeries<'a>) -> Series), but we don't have this(Perhaps we don't want to implement it either, after all, Series may contain multiple arrays if not re-chunk).
There was a problem hiding this comment.
Ok, let's leave it for now. Maybe we can take a look later. Want to have this in the release. :)
No description provided.