From e93998d9b40c4295fe40f47d1da2cd3c6c877fc3 Mon Sep 17 00:00:00 2001 From: MarkusBonsch Date: Thu, 10 May 2018 19:17:53 +0200 Subject: [PATCH 1/4] Fixed bug #2829 --- NEWS.md | 2 +- inst/tests/tests.Rraw | 5 +++++ src/assign.c | 11 +++++++---- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index e12a8a54cd..87f479ee21 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,7 +4,7 @@ ### Changes in v1.11.3 (in development) #### BUG FIXES - +1. Fixed bug where assignment with 0 matching rows threw error [#2829](https://github.com/Rdatatable/data.table/issues/2829). Thanks to @cguill95 for reporting and to @MarkusBonsch for fixing. #### NOTES diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index ce654dde1e..6cb53e90f4 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -11788,6 +11788,11 @@ test(1909.6, nchar(ans$V2), INT(3287,3287,3287)) DT = data.table(x=1:10, y=1:2) test(1910, DT[, v:=cumsum(x), by="y"], data.table(x=1:10, y=1:2, v=INT(1,2,4,6,9,12,16,20,25,30))) +# testing issue #2829 (assigning to 0 rows) +DT = data.table(COL_INT = 1L, COL_INT_2 = 5L) +test(1911, + DT[COL_INT == 0L, c("COL_INT", "NEW_COL"):=list(COL_INT_2, "Test")], + data.table(COL_INT = 1L, COL_INT_2 = 5L, NEW_COL = NA_character_)) ################################### # Add new tests above this line # diff --git a/src/assign.c b/src/assign.c index c919294ef7..53d9d76010 100644 --- a/src/assign.c +++ b/src/assign.c @@ -277,7 +277,7 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v // newcolnames : add these columns (if any) // cols : column names or numbers corresponding to the values to set // rows : row numbers to assign - R_len_t i, j, nrow, targetlen, vlen, r, oldncol, oldtncol, coln, protecti=0, newcolnum, indexLength; + R_len_t i, j, nrow, numToDo, targetlen, vlen, r, oldncol, oldtncol, coln, protecti=0, newcolnum, indexLength; SEXP targetcol, RHS, names, nullint, thisvalue, thisv, targetlevels, newcol, s, colnam, class, tmp, colorder, key, index, a, assignedNames, indexNames; SEXP bindingIsLocked = getAttrib(dt, install(".data.table.locked")); Rboolean verbose = LOGICAL(verb)[0], anytodelete=FALSE, isDataTable=FALSE; @@ -316,6 +316,7 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v if (oldncol<1) error("Cannot use := to add columns to a null data.table (no columns), currently. You can use := to add (empty) columns to a 0-row data.table (1 or more empty columns), though."); nrow = length(VECTOR_ELT(dt,0)); if (isNull(rows)) { + numToDo = nrow; targetlen = nrow; if (verbose) Rprintf("Assigning to all %d rows\n", nrow); // fast way to assign to whole column, without creating 1:nrow(x) vector up in R, or here in C @@ -328,7 +329,7 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v if (!isInteger(rows)) error("i is type '%s'. Must be integer, or numeric is coerced with warning. If i is a logical subset, simply wrap with which(), and take the which() outside the loop if possible for efficiency.", type2char(TYPEOF(rows))); targetlen = length(rows); - int numToDo = 0; + numToDo = 0; for (i=0; inrow) error("i[%d] is %d which is out of range [1,nrow=%d].",i+1,INTEGER(rows)[i],nrow); @@ -408,8 +409,10 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v else colnam = STRING_ELT(newcolnames,coln-length(names)); if (coln+1 <= oldncol && isNull(thisvalue)) continue; // delete existing column(s) afterwards, near end of this function if (vlen<1 && nrow>0) { - if (coln+1 <= oldncol) { - error("RHS of assignment to existing column '%s' is zero length but not NULL. If you intend to delete the column use NULL. Otherwise, the RHS must have length > 0; e.g., NA_integer_. If you are trying to change the column type to be an empty list column then, as with all column type changes, provide a full length RHS vector such as vector('list',nrow(DT)); i.e., 'plonk' in the new column.", CHAR(STRING_ELT(names,coln))); + if (coln+1 <= oldncol) { + if(numToDo > 0){ //fixes #2829 + error("RHS of assignment to existing column '%s' is zero length but not NULL. If you intend to delete the column use NULL. Otherwise, the RHS must have length > 0; e.g., NA_integer_. If you are trying to change the column type to be an empty list column then, as with all column type changes, provide a full length RHS vector such as vector('list',nrow(DT)); i.e., 'plonk' in the new column.", CHAR(STRING_ELT(names,coln))); + } } else if (TYPEOF(thisvalue)!=VECSXP) { // list() is ok for new columns newcolnum = coln-length(names); if (newcolnum<0 || newcolnum>=length(newcolnames)) From 5c2b73ce08d425034c60c63f6aa8eb09b9c1cceb Mon Sep 17 00:00:00 2001 From: MarkusBonsch Date: Thu, 10 May 2018 21:02:20 +0200 Subject: [PATCH 2/4] Added test to increase coverage. --- inst/tests/tests.Rraw | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index 6cb53e90f4..bf03ca0ce8 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -11790,9 +11790,12 @@ test(1910, DT[, v:=cumsum(x), by="y"], data.table(x=1:10, y=1:2, v=INT(1,2,4,6,9 # testing issue #2829 (assigning to 0 rows) DT = data.table(COL_INT = 1L, COL_INT_2 = 5L) -test(1911, +test(1911.1, DT[COL_INT == 0L, c("COL_INT", "NEW_COL"):=list(COL_INT_2, "Test")], data.table(COL_INT = 1L, COL_INT_2 = 5L, NEW_COL = NA_character_)) +test(1911.2, + DT[, COL_INT := integer(0)], + error = "RHS of assignment to existing column 'COL_INT' is zero length but not NULL.*") ################################### # Add new tests above this line # From 7251a22212364f91b6144feb4f093a14ebe669e4 Mon Sep 17 00:00:00 2001 From: MarkusBonsch Date: Sat, 12 May 2018 09:17:29 +0200 Subject: [PATCH 3/4] Adapted to @mattdowle's review --- NEWS.md | 3 ++- src/assign.c | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index 87f479ee21..45c356e684 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,7 +4,8 @@ ### Changes in v1.11.3 (in development) #### BUG FIXES -1. Fixed bug where assignment with 0 matching rows threw error [#2829](https://github.com/Rdatatable/data.table/issues/2829). Thanks to @cguill95 for reporting and to @MarkusBonsch for fixing. +1. Empty RHS of := is no longer an error when the i clause returns no rows to assign to anyway, [#2829](https://github.com/Rdatatable/data.table/issues/2829). Thanks to @cguill95 for reporting and to @MarkusBonsch for fixing. + #### NOTES diff --git a/src/assign.c b/src/assign.c index 53d9d76010..a4cb2d3a5e 100644 --- a/src/assign.c +++ b/src/assign.c @@ -409,11 +409,9 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v else colnam = STRING_ELT(newcolnames,coln-length(names)); if (coln+1 <= oldncol && isNull(thisvalue)) continue; // delete existing column(s) afterwards, near end of this function if (vlen<1 && nrow>0) { - if (coln+1 <= oldncol) { - if(numToDo > 0){ //fixes #2829 + if (coln+1 <= oldncol && numToDo > 0) { // numToDo > 0 fixes #2829, see test 1911 error("RHS of assignment to existing column '%s' is zero length but not NULL. If you intend to delete the column use NULL. Otherwise, the RHS must have length > 0; e.g., NA_integer_. If you are trying to change the column type to be an empty list column then, as with all column type changes, provide a full length RHS vector such as vector('list',nrow(DT)); i.e., 'plonk' in the new column.", CHAR(STRING_ELT(names,coln))); - } - } else if (TYPEOF(thisvalue)!=VECSXP) { // list() is ok for new columns + } else if (coln+1 > oldncol && TYPEOF(thisvalue)!=VECSXP) { // list() is ok for new columns newcolnum = coln-length(names); if (newcolnum<0 || newcolnum>=length(newcolnames)) error("Internal logical error. length(newcolnames)=%d, length(names)=%d, coln=%d", length(newcolnames), length(names), coln); From 46a4512654c833abcb8fba4d0aeac11b5d0c990a Mon Sep 17 00:00:00 2001 From: MarkusBonsch Date: Sat, 12 May 2018 19:33:11 +0200 Subject: [PATCH 4/4] Formatted according to @jangorecki's review. --- NEWS.md | 1 + src/assign.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 45c356e684..afbc57a619 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,7 @@ ### Changes in v1.11.3 (in development) #### BUG FIXES + 1. Empty RHS of := is no longer an error when the i clause returns no rows to assign to anyway, [#2829](https://github.com/Rdatatable/data.table/issues/2829). Thanks to @cguill95 for reporting and to @MarkusBonsch for fixing. #### NOTES diff --git a/src/assign.c b/src/assign.c index a4cb2d3a5e..1f5d9161a7 100644 --- a/src/assign.c +++ b/src/assign.c @@ -410,7 +410,7 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v if (coln+1 <= oldncol && isNull(thisvalue)) continue; // delete existing column(s) afterwards, near end of this function if (vlen<1 && nrow>0) { if (coln+1 <= oldncol && numToDo > 0) { // numToDo > 0 fixes #2829, see test 1911 - error("RHS of assignment to existing column '%s' is zero length but not NULL. If you intend to delete the column use NULL. Otherwise, the RHS must have length > 0; e.g., NA_integer_. If you are trying to change the column type to be an empty list column then, as with all column type changes, provide a full length RHS vector such as vector('list',nrow(DT)); i.e., 'plonk' in the new column.", CHAR(STRING_ELT(names,coln))); + error("RHS of assignment to existing column '%s' is zero length but not NULL. If you intend to delete the column use NULL. Otherwise, the RHS must have length > 0; e.g., NA_integer_. If you are trying to change the column type to be an empty list column then, as with all column type changes, provide a full length RHS vector such as vector('list',nrow(DT)); i.e., 'plonk' in the new column.", CHAR(STRING_ELT(names,coln))); } else if (coln+1 > oldncol && TYPEOF(thisvalue)!=VECSXP) { // list() is ok for new columns newcolnum = coln-length(names); if (newcolnum<0 || newcolnum>=length(newcolnames))