Skip to content

Early exit for vec_in() and vec_matches() when one input is empty #1636

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Early exit for vec_in() and vec_matches() when one input is empty #1636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/dictionary.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,19 @@ SEXP vec_match_params(SEXP needles,
DF_FALLBACK_quiet,
&_);
PROTECT_N(type, &nprot);
// can exit early here. Following operations cannot fail:
// * `vec_cast()` must work
Copy link
Member

@lionel- lionel- Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a chance that vec_cast() might fail, e.g. in case of lossy coercions.

Copy link
Contributor Author

@mgirlich mgirlich Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought vec_ptype2() returns the richer type so that we can safely cast. In particular I would not expect to get a lossy coercion there. Is this not the case?

Copy link
Member

@lionel- lionel- Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right in general, at least for lossy coercions. But in principle there might be types where casting to the common type might fail. E.g. a strict factor type that where ptype(strict_factor, character) == strict_factor, and then vec_cast() might fail.

That said, it might be a good reason to decide that strict types shouldn't be coercible this way, and either fail (making it less practical for users) or lose their strictness (but that's counter to the idea of a safe strict type).

// * `vec_proxy_equal()`
// * `vec_normalize_encoding()`
if (vec_size(needles) == 0) {
UNPROTECT(nprot);
return vctrs_shared_empty_int;
}

if (vec_size(haystack) == 0) {
UNPROTECT(nprot);
return vec_recycle(vctrs_shared_na_int, vec_size(needles));
}

needles = vec_cast_params(needles, type,
needles_arg, vec_args.empty,
Expand Down Expand Up @@ -479,6 +492,19 @@ SEXP vctrs_in(SEXP needles, SEXP haystack, SEXP na_equal_, SEXP frame) {
DF_FALLBACK_quiet,
&_);
PROTECT_N(type, &nprot);
// can exit early here. Following operations cannot fail:
// * `vec_cast()` must work
// * `vec_proxy_equal()`
// * `vec_normalize_encoding()`
if (vec_size(needles) == 0) {
UNPROTECT(nprot);
return vctrs_shared_empty_lgl;
}

if (vec_size(haystack) == 0) {
UNPROTECT(nprot);
return vec_recycle(vctrs_shared_false, vec_size(needles));
}

needles = vec_cast_params(needles, type,
&needles_arg, vec_args.empty,
Expand Down
4 changes: 4 additions & 0 deletions src/utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,7 @@ SEXP vctrs_shared_false = NULL;

Rcomplex vctrs_shared_na_cpl;
SEXP vctrs_shared_na_lgl = NULL;
SEXP vctrs_shared_na_int = NULL;
SEXP vctrs_shared_na_list = NULL;

SEXP vctrs_shared_zero_int = NULL;
Expand Down Expand Up @@ -1829,6 +1830,9 @@ void vctrs_init_utils(SEXP ns) {
vctrs_shared_na_lgl = r_new_shared_vector(LGLSXP, 1);
LOGICAL(vctrs_shared_na_lgl)[0] = NA_LOGICAL;

vctrs_shared_na_int = r_new_shared_vector(INTSXP, 1);
INTEGER(vctrs_shared_na_int)[0] = NA_INTEGER;

vctrs_shared_na_list = r_new_shared_vector(VECSXP, 1);
SET_VECTOR_ELT(vctrs_shared_na_list, 0, R_NilValue);

Expand Down
1 change: 1 addition & 0 deletions src/vctrs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ extern SEXP vctrs_shared_false;

extern Rcomplex vctrs_shared_na_cpl;
extern SEXP vctrs_shared_na_lgl;
extern SEXP vctrs_shared_na_int;
extern SEXP vctrs_shared_na_list;

SEXP vec_unspecified(R_len_t n);
Expand Down
31 changes: 31 additions & 0 deletions tests/testthat/_snaps/dictionary.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,34 @@
Error in `vec_in()`:
! Can't combine `n$x$foo` <double> and `h$x$foo` <character>.

# vec_match() and vec_in() work for empty vectors

Code
(expect_error(vctrs::vec_in(1:2, character()), class = "vctrs_error_incompatible_type")
)
Output
<error/vctrs_error_incompatible_type>
Error in `vctrs::vec_in()`:
! Can't combine <integer> and <character>.
Code
(expect_error(vctrs::vec_in(character(), 1:2), class = "vctrs_error_incompatible_type")
)
Output
<error/vctrs_error_incompatible_type>
Error in `vctrs::vec_in()`:
! Can't combine <character> and <integer>.
Code
(expect_error(vctrs::vec_match(1:2, character()), class = "vctrs_error_incompatible_type")
)
Output
<error/vctrs_error_incompatible_type>
Error in `vctrs::vec_match()`:
! Can't combine <integer> and <character>.
Code
(expect_error(vctrs::vec_match(character(), 1:2), class = "vctrs_error_incompatible_type")
)
Output
<error/vctrs_error_incompatible_type>
Error in `vctrs::vec_match()`:
! Can't combine <character> and <integer>.

15 changes: 15 additions & 0 deletions tests/testthat/test-dictionary.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,21 @@ test_that("vec_match works with empty data frame", {
expect_equal(out, vec_init(integer(), 3))
})

test_that("vec_match() and vec_in() work for empty vectors", {
expect_equal(vctrs::vec_in(1:2, integer()), c(FALSE, FALSE))
expect_equal(vctrs::vec_in(integer(), 1:2), logical())

expect_equal(vctrs::vec_match(1:2, integer()), c(NA_integer_, NA_integer_))
expect_equal(vctrs::vec_match(integer(), 1:2), integer())

expect_snapshot({
(expect_error(vctrs::vec_in(1:2, character()), class = "vctrs_error_incompatible_type"))
(expect_error(vctrs::vec_in(character(), 1:2), class = "vctrs_error_incompatible_type"))
(expect_error(vctrs::vec_match(1:2, character()), class = "vctrs_error_incompatible_type"))
(expect_error(vctrs::vec_match(character(), 1:2), class = "vctrs_error_incompatible_type"))
})
})

test_that("matching functions take the equality proxy (#375)", {
local_comparable_tuple()
x <- tuple(c(1, 2, 1), 1:3)
Expand Down