-
Notifications
You must be signed in to change notification settings - Fork 66
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
Clean up slice fallback and remove bit64 slice hacks #1707
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -296,6 +296,10 @@ static SEXP chop_shaped(SEXP x, SEXP indices, struct vctrs_chop_info info) { | |
} | ||
|
||
static SEXP chop_fallback(SEXP x, SEXP indices, struct vctrs_chop_info info) { | ||
// TODO: Do we really care about micro performance that much here? Can we just | ||
// merge with `chop_shaped_fallback()` now that `vec_slice_fallback()` | ||
// handles shaped an unshaped vectors? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This I can't imagine we care about the performance of those cases that much. I think I'd rather simplify this code by a lot by merging it with |
||
|
||
// Evaluate in a child of the global environment to allow dispatch | ||
// to custom functions. We define `[` to point to its base | ||
// definition to ensure consistent look-up. This is the same logic | ||
|
@@ -306,16 +310,8 @@ static SEXP chop_fallback(SEXP x, SEXP indices, struct vctrs_chop_info info) { | |
Rf_defineVar(syms_i, info.index, env); | ||
|
||
// Construct call with symbols, not values, for performance. | ||
// TODO - Remove once bit64 is updated on CRAN. Special casing integer64 | ||
// objects to ensure correct slicing with `NA_integer_`. | ||
SEXP call; | ||
if (is_integer64(x)) { | ||
call = PROTECT(Rf_lang3(syms.vec_slice_dispatch_integer64, syms_x, syms_i)); | ||
Rf_defineVar(syms.vec_slice_dispatch_integer64, fns.vec_slice_dispatch_integer64, env); | ||
} else { | ||
call = PROTECT(Rf_lang3(syms_bracket, syms_x, syms_i)); | ||
Rf_defineVar(syms_bracket, fns_bracket, env); | ||
} | ||
SEXP call = PROTECT(Rf_lang3(syms_bracket, syms_x, syms_i)); | ||
Rf_defineVar(syms_bracket, fns_bracket, env); | ||
|
||
for (R_len_t i = 0; i < info.out_size; ++i) { | ||
if (info.has_indices) { | ||
|
@@ -329,6 +325,7 @@ static SEXP chop_fallback(SEXP x, SEXP indices, struct vctrs_chop_info info) { | |
|
||
SEXP elt = PROTECT(Rf_eval(call, env)); | ||
|
||
// Same logic as `vec_slice_fallback()` | ||
if (!vec_is_restored(elt, x)) { | ||
elt = vec_restore(elt, x, vec_owned(elt)); | ||
} | ||
|
@@ -349,7 +346,6 @@ static r_obj* chop_fallback_shaped(r_obj* x, r_obj* indices, struct vctrs_chop_i | |
++(*info.p_index); | ||
} | ||
|
||
// `vec_slice_fallback()` will also `vec_restore()` for us | ||
r_obj* elt = PROTECT(vec_slice_fallback(x, info.index)); | ||
|
||
SET_VECTOR_ELT(info.out, i, elt); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,34 +207,39 @@ r_obj* df_slice(r_obj* x, r_obj* subscript) { | |
return out; | ||
} | ||
|
||
static | ||
r_obj* bracket_dispatch(r_obj* x, r_obj* subscript) { | ||
return vctrs_dispatch2( | ||
syms_bracket, fns_bracket, | ||
syms_x, x, | ||
syms_i, subscript | ||
); | ||
} | ||
static | ||
r_obj* bracket_shaped_dispatch(r_obj* x, r_obj* subscript) { | ||
return vctrs_dispatch2( | ||
syms.bracket_shaped_dispatch, fns.bracket_shaped_dispatch, | ||
syms_x, x, | ||
syms_i, subscript | ||
); | ||
} | ||
Comment on lines
+210
to
+225
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now have these nice two parallel functions. |
||
|
||
r_obj* vec_slice_fallback(r_obj* x, r_obj* subscript) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The point of It first hands off to |
||
// TODO - Remove once bit64 is updated on CRAN. Special casing integer64 | ||
// objects to ensure correct slicing with `NA_integer_`. | ||
if (is_integer64(x)) { | ||
return vctrs_dispatch2(syms.vec_slice_fallback_integer64, fns.vec_slice_fallback_integer64, | ||
syms_x, x, | ||
syms_i, subscript); | ||
} | ||
r_obj* out = r_null; | ||
|
||
return vctrs_dispatch2(syms.vec_slice_fallback, fns.vec_slice_fallback, | ||
syms_x, x, | ||
syms_i, subscript); | ||
} | ||
if (has_dim(x)) { | ||
out = KEEP(bracket_shaped_dispatch(x, subscript)); | ||
} else { | ||
out = KEEP(bracket_dispatch(x, subscript)); | ||
} | ||
|
||
static | ||
r_obj* vec_slice_dispatch(r_obj* x, r_obj* subscript) { | ||
// TODO - Remove once bit64 is updated on CRAN. Special casing integer64 | ||
// objects to ensure correct slicing with `NA_integer_`. | ||
if (is_integer64(x)) { | ||
return vctrs_dispatch2(syms.vec_slice_dispatch_integer64, fns.vec_slice_dispatch_integer64, | ||
syms_x, x, | ||
syms_i, subscript); | ||
// Take over attribute restoration only if there is no `[` method | ||
if (!vec_is_restored(out, x)) { | ||
out = vec_restore(out, x, vec_owned(out)); | ||
} | ||
|
||
return vctrs_dispatch2(syms_bracket, fns_bracket, | ||
syms_x, x, | ||
syms_i, subscript); | ||
FREE(1); | ||
return out; | ||
} | ||
|
||
bool vec_requires_fallback(r_obj* x, struct vctrs_proxy_info info) { | ||
|
@@ -300,18 +305,7 @@ r_obj* vec_slice_unsafe(r_obj* x, r_obj* subscript) { | |
subscript = KEEP_N(compact_materialize(subscript), &nprot); | ||
} | ||
|
||
r_obj* out; | ||
|
||
if (has_dim(x)) { | ||
out = KEEP_N(vec_slice_fallback(x, subscript), &nprot); | ||
} else { | ||
out = KEEP_N(vec_slice_dispatch(x, subscript), &nprot); | ||
} | ||
|
||
// Take over attribute restoration only if there is no `[` method | ||
if (!vec_is_restored(out, x)) { | ||
out = vec_restore(out, x, vec_owned(out)); | ||
} | ||
r_obj* out = vec_slice_fallback(x, subscript); | ||
|
||
FREE(nprot); | ||
return out; | ||
|
@@ -381,11 +375,15 @@ bool vec_is_restored(r_obj* x, r_obj* to) { | |
return false; | ||
} | ||
|
||
// Class is restored if it contains any other attributes than names. | ||
// We might want to add support for data frames later on. | ||
// Class is restored if it contains any other attributes than names, dim, or | ||
// dimnames. We might want to add support for data frames later on. | ||
r_obj* node = attrib; | ||
while (node != r_null) { | ||
if (r_node_tag(node) == r_syms.names) { | ||
r_obj* tag = r_node_tag(node); | ||
|
||
if (tag == r_syms.names || | ||
tag == r_syms.dim || | ||
tag == r_syms.dim_names) { | ||
Comment on lines
-388
to
+386
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
node = r_node_cdr(node); | ||
continue; | ||
} | ||
|
@@ -482,11 +480,6 @@ r_obj* ffi_slice_rep(r_obj* x, r_obj* ffi_i, r_obj* ffi_n) { | |
|
||
|
||
void vctrs_init_slice(r_obj* ns) { | ||
syms.vec_slice_dispatch_integer64 = r_sym("vec_slice_dispatch_integer64"); | ||
syms.vec_slice_fallback = r_sym("vec_slice_fallback"); | ||
syms.vec_slice_fallback_integer64 = r_sym("vec_slice_fallback_integer64"); | ||
|
||
fns.vec_slice_dispatch_integer64 = r_eval(syms.vec_slice_dispatch_integer64, ns); | ||
fns.vec_slice_fallback = r_eval(syms.vec_slice_fallback, ns); | ||
fns.vec_slice_fallback_integer64 = r_eval(syms.vec_slice_fallback_integer64, ns); | ||
syms.bracket_shaped_dispatch = r_sym("bracket_shaped_dispatch"); | ||
fns.bracket_shaped_dispatch = r_eval(syms.bracket_shaped_dispatch, ns); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,18 @@ test_that("vec_slice() unclasses input before calling `vec_restore()`", { | |
}) | ||
|
||
test_that("can call `vec_slice()` from `[` methods with shaped objects without infloop", { | ||
skip("Until infloop is talked about") | ||
# TODO: I don't think it is our job to prevent this. | ||
# You can already make this infloop today with non-shaped objects, i.e | ||
# `[.vctrs_foobar` = function(x, i, ...) vec_slice(x, i) | ||
# x <- structure(1:4, class = "vctrs_foobar") | ||
# x[1] | ||
# So we shouldn't be special casing shaped objects. | ||
# I believe that if your `[` method is going to call `vec_slice()` (i.e. you | ||
# are requesting native vctrs slicing), then you need to declare a | ||
# `vec_proxy()` method as well to tell vctrs what it needs to be natively | ||
# slicing. | ||
|
||
local_methods( | ||
`[.vctrs_foobar` = function(x, i, ...) vec_slice(x, i) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so lets talk about this. This infloop check is for:
In that case, I don't think that avoiding this infloop is our responsibility. The way we tried to do this was by calling It it also 100% possible to do this already with non-shaped vectors. i.e. this is with CRAN vctrs: library(vctrs)
`[.vctrs_foobar` <- function(x, i, ...) vec_slice(x, i)
x <- structure(1:4, class = "vctrs_foobar")
# infloop
x[1] I think that if you are telling vctrs that you want |
||
) | ||
|
@@ -351,6 +363,7 @@ test_that("vec_restore() is called after slicing data frames", { | |
|
||
test_that("additional subscripts are forwarded to `[`", { | ||
local_methods( | ||
vec_proxy.vctrs_foobar = function(x, ...) x, | ||
`[.vctrs_foobar` = function(x, i, ...) vec_index(x, i, ...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise produces the infloop from before, but we are just trying to test |
||
) | ||
|
||
|
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.
The intention of this
vec_slice_fallback()
path was to handle shaped fallback vectors when callingvec_slice()
. It was intended to give the[
methods for shaped vectors a chance to run, which we'd then restore on the backside if needed withvec_is_restored()
andvec_restore()
(because base[
methods drop the class).However, we were unclassing the input before calling
[
, which kind of defeats the whole point!This was a problem with bit64, because the
[.integer64
method is required to correctly handleNA_integer_
indices.Created on 2022-09-30 by the reprex package (v2.0.1)
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.
Additionally, this function does not need to proxy/restore, because if a
vec_proxy()
method was defined then we would not have gone through the fallback path to begin with!I've trimmed it down to only calling
[
, which feels more correct.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.
Lastly, it was given a rather unfortunate name, because it made us think this was the fallback slice method that we should be using everywhere, when really it was just intended for shaped vectors.
It was actually being used in other places, like the fallback paths of
list_unchop()
andvec_chop()
, when really we should have been using a more generic slice fallback function that actually handles that "maybe restore" part on the backside.I'm fairly certain this was the reason we tried to make it unclass/proxy/restore all in the same function.
It now has the name
bracket_shaped_dispatch()
to go along with the C levelbracket_dispatch()
which callsx[i]
in the non-shaped case.