Fixing bug on assigning with 0 row matches #2860
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2860 +/- ##
==========================================
+ Coverage 93.46% 93.47% +<.01%
==========================================
Files 61 61
Lines 12352 12355 +3
==========================================
+ Hits 11545 11549 +4
+ Misses 807 806 -1
Continue to review full report at Codecov.
|
NEWS.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
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]..."
There was a problem hiding this comment.
Perfect, thanks! I have changed it in my latest commit.
src/assign.c
Outdated
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
AppVeyor complains about memory exhausted at test 1879.6. I guess this has nothing to do with my PR, but I don't know, how to retrigger the tests. |
|
@MarkusBonsch you have to access appveyor, when creating account and using authentication with github you should have permission to restart builds. |
src/assign.c
Outdated
| 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 | ||
| 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))); |
NEWS.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
empty line should separate header and first item
Closes #2829.
assign.cwas modified to only throw error on 0 length RHS if>0rows are to be updated.