Skip to content
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

Fixing bug on assigning with 0 row matches #2860

Merged
merged 4 commits into from
May 12, 2018
Merged

Fixing bug on assigning with 0 row matches #2860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e93998d
Fixed bug #2829
MarkusBonsch May 10, 2018
5c2b73c
Added test to increase coverage.
MarkusBonsch May 10, 2018
7251a22
Adapted to @mattdowle's review
MarkusBonsch May 12, 2018
46a4512
Formatted according to @jangorecki's review.
MarkusBonsch May 12, 2018
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
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

#### 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


Expand Down
8 changes: 8 additions & 0 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11788,6 +11788,14 @@ 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.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 #
Expand Down
9 changes: 5 additions & 4 deletions src/assign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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; i<targetlen; i++) {
if ((INTEGER(rows)[i]<0 && INTEGER(rows)[i]!=NA_INTEGER) || INTEGER(rows)[i]>nrow)
error("i[%d] is %d which is out of range [1,nrow=%d].",i+1,INTEGER(rows)[i],nrow);
Expand Down Expand Up @@ -408,9 +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 (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);
Expand Down