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 2 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: 1 addition & 1 deletion NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

How about this to reflect the nuance : "Empty RHS of := is no longer an error when the i clause returns no rows to assign to anyway, [#2829]..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, thanks! I have changed it in my latest commit.

#### 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
11 changes: 7 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,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
Copy link
Member

@mattdowle mattdowle May 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It took me a while to grok numToDo, but yes. Looks good. Could it be one if rather than two?
if (coln+1<=oldncol && numToDo>0) { // numToDo fixes #2829. See test 1911

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be changed, but then the else if statement below needs modification: else if(coln+1 > oldncol && TYPEOF(thisvalue)!=VECSXP). I think, this makes sense and have implemented it in the latest commit.

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)));
Copy link
Member

Choose a reason for hiding this comment

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

we use 2 spaces indention

}
} else if (TYPEOF(thisvalue)!=VECSXP) { // list() is ok for new columns
newcolnum = coln-length(names);
if (newcolnum<0 || newcolnum>=length(newcolnames))
Expand Down