Skip to content

Commit

Permalink
Retain earlier dcast(fill=list(...)) behavior relying on base coercio…
Browse files Browse the repository at this point in the history
…n behavior for lists (#6051)

* Retain fill=list(...) behavior

* refactor to unclutter line for typical usage

* test list->int64 coercion too
  • Loading branch information
MichaelChirico authored Apr 10, 2024
1 parent 585ec52 commit 26c558d
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 7 deletions.
7 changes: 6 additions & 1 deletion inst/tests/tests.Rraw
Original file line number Diff line number Diff line change
Expand Up @@ -18314,7 +18314,12 @@ test(2247.4, split(dt, ~y+z), list("a.c"=dt[1], "b.c"=dt[2], "a.d"=dt[3], "b.d"=
if (test_bit64) {
i64v = as.integer64(c(12345678901234, 70, 20, NA))
apple = data.table(id = c("a", "b", "b"), time = c(1L, 1L, 2L), y = i64v[1:3])
test(2248, dcast(apple, id ~ time, value.var = "y"), data.table(id = c('a', 'b'), `1` = i64v[1:2], `2` = i64v[4:3], key='id'))
test(2248.1, dcast(apple, id ~ time, value.var = "y"), ans<-data.table(id = c('a', 'b'), `1` = i64v[1:2], `2` = i64v[4:3], key='id'))
# associated regression test: downtreams used fill=list() which is not directly supported by coerceAs()
DT = data.table(a=1:2, b=2:3, c=3)
test(2248.2, dcast(DT, a ~ b, value.var='c', fill=list(0L)), data.table(a=1:2, `2`=c(3, 0), `3`=c(0, 3), key='a'))
# also ensure list() gets coerced to integer64 correctly
test(2248.3, dcast(apple, id ~ time, value.var = "y", fill=list(NA)), ans)
}

# Unit tests for DT[, .SD] retaining secondary indices, #1709
Expand Down
13 changes: 8 additions & 5 deletions src/fcast.c
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,17 @@ SEXP fcast(SEXP lhs, SEXP val, SEXP nrowArg, SEXP ncolArg, SEXP idxArg, SEXP fil
SEXP thisfill = fill;
const SEXPTYPE thistype = TYPEOF(thiscol);
int nprotect = 0;
if(some_fill){
if (some_fill) {
if (isNull(fill)) {
if (LOGICAL(is_agg)[0]) {
thisfill = PROTECT(allocNAVector(thistype, 1)); nprotect++;
} else thisfill = VECTOR_ELT(fill_d, i);
if (LOGICAL(is_agg)[0]) {
thisfill = PROTECT(allocNAVector(thistype, 1)); nprotect++;
} else
thisfill = VECTOR_ELT(fill_d, i);
}
if (isVectorAtomic(thiscol)) { // defer error handling to below, but also skip on list
thisfill = PROTECT(coerceAs(thisfill, thiscol, /*copyArg=*/ScalarLogical(false))); nprotect++;
// #5980: some callers used fill=list(...) and relied on R's coercion mechanics for lists, which are nontrivial, so just dispatch and double-coerce.
if (isNewList(thisfill)) { thisfill = PROTECT(coerceVector(thisfill, TYPEOF(thiscol))); nprotect++; }
thisfill = PROTECT(coerceAs(thisfill, thiscol, /*copyArg=*/ScalarLogical(false))); nprotect++;
}
}
switch (thistype) {
Expand Down
2 changes: 1 addition & 1 deletion src/utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ SEXP coerceUtf8IfNeeded(SEXP x) {
return(ans);
}

// class1 is used by coerseAs only, which is used by frollR.c and nafill.c only
// class1 is used by coerceAs only, which is used by frollR.c and nafill.c only
const char *class1(SEXP x) {
SEXP cl = getAttrib(x, R_ClassSymbol);
if (length(cl))
Expand Down

0 comments on commit 26c558d

Please sign in to comment.