diff --git a/NEWS.md b/NEWS.md index 381d4f82..4d48a286 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # collapse 2.1.7 +* Fixed a bug in `fmatch()` (and thus `%in%`/`%!in%`/`%iin%`/`%!iin%` and joins) where a logical `NA` in `x` could spuriously match a non-`NA` value in `table` (e.g. `2L`) when `table` was not itself logical. Thanks @LJ-Jenkins for reporting (#870). + * Fixed a bug in `fslice()` (grouped, `n = 1`, `with.ties = FALSE`) that caused R to crash with a fatal error when a group had only missing values in `order.by`. Thanks @chihyunkim for reporting (#867). * The *collapse* article is now published in the Journal of Statistical Software: https://doi.org/10.18637/jss.v116.i01. This article is now the primary citation for academic use of *collapse*. It is also a great reference to quickly and thoroughly understand the package. `citation("collapse")` was also updated in this regard. The APA-style citation is: diff --git a/src/match.c b/src/match.c index fc73bb6d..71c37272 100644 --- a/src/match.c +++ b/src/match.c @@ -152,7 +152,8 @@ SEXP match_single(SEXP x, SEXP table, SEXP nomatch) { } else goto bigint; anyNA = !(inherits(x, "na.included") && inherits(table, "na.included")); } else if (tx == LGLSXP) { - M = 3; + if(TYPEOF(table) == LGLSXP) M = 3; + else { tx = INTSXP; goto bigint; } // table is not logical: cannot assume values are restricted to {0, 1, NA}, need generic hashing to avoid spurious NA collisions (#870) } else error("Type %s is not supported.", type2char(tx)); int *restrict h = (int*)R_Calloc(M, int); // Table to save the hash values, table has size M diff --git a/tests/testthat/test-fmatch.R b/tests/testthat/test-fmatch.R index 35f41eae..5774676e 100644 --- a/tests/testthat/test-fmatch.R +++ b/tests/testthat/test-fmatch.R @@ -178,3 +178,28 @@ test_that("fmatch works with logical data", { expect_equal(fmatch(FALSE, x), 2L) }) +test_that("fmatch does not match logical NA in x to non-logical values in table (#870)", { + table_int <- c(0L, 1L, 2L) + expect_identical(fmatch(c(FALSE, TRUE, NA), table_int), match(c(FALSE, TRUE, NA), table_int)) + expect_identical(fmatch(NA, table_int), NA_integer_) + expect_identical(NA %iin% table_int, integer(0)) + + # Larger table where the colliding value is at a different position + table_int2 <- c(5L, 2L, 0L, 1L, 9L) + expect_identical(fmatch(c(FALSE, TRUE, NA), table_int2), match(c(FALSE, TRUE, NA), table_int2)) + + # table actually containing NA_integer_: NA in x should match it + table_int3 <- c(0L, 1L, NA_integer_) + expect_identical(fmatch(c(FALSE, TRUE, NA), table_int3), match(c(FALSE, TRUE, NA), table_int3)) + + # Reverse direction (already worked prior to the fix) + expect_identical(fmatch(table_int, c(FALSE, TRUE, NA)), match(table_int, c(FALSE, TRUE, NA))) + + # table is double + table_dbl <- c(0, 1, 2) + expect_identical(fmatch(c(FALSE, TRUE, NA), table_dbl), match(c(FALSE, TRUE, NA), table_dbl)) + + # genuine logical-vs-logical matching still works, including NA matching NA + expect_identical(fmatch(c(FALSE, TRUE, NA), c(TRUE, NA, FALSE)), match(c(FALSE, TRUE, NA), c(TRUE, NA, FALSE))) +}) +