From 71fe545c690d34bb13ba411f921b33eb4ec68c6b Mon Sep 17 00:00:00 2001 From: MarkusBonsch Date: Sat, 6 Jan 2018 21:05:54 +0100 Subject: [PATCH 1/2] Fixed bug with assignments like DT[,c('x', 'y') := 1:10] --- NEWS.md | 2 ++ inst/tests/tests.Rraw | 1 + src/assign.c | 4 +++- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 4e45a33ce4..dfec507cf5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -117,6 +117,8 @@ 28. CJ() works with multiple empty vectors now [#2511](https://github.com/Rdatatable/data.table/issues/2511). Thanks to @MarkusBonsch for fixing. +29. `:=` assignment of one vector to two or more columns, e.g. DT[, c("x", "y") := 1:10], caused a tricky error if `setnames` was called afterwards because the code failed to copy the vector. [#2540](https://github.com/Rdatatable/data.table/issues/2540). This is an old issue [#185](https://github.com/Rdatatable/data.table/issues/185) that had been fixed but reappeared when code was refactored. Thanks to @patrickhowerter for the report and the reproducible example and to @MarkusBonsch for fixing. + #### NOTES diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index 9be9995dde..b9f57b8966 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -2151,6 +2151,7 @@ test(752, DT[,head(.SD,2)[,new:=1:.N],by=a], data.table(a=rep(1:3,each=2),b=c(1: # Test duplicate() of recycled plonking RHS, #2298 DT = data.table(a=letters[3:1],x=1:3) test(753, setkey(DT[,c("x1","x2"):=x],a), data.table(a=letters[1:3],x=3:1,x1=3:1,x2=3:1,key="a")) +test(753.1, DT[,c("x1","x2"):=3:1, verbose = TRUE], output = "RHS for item 2 has been duplicated") DT = data.table(a=letters[3:1],x=1:3,y=4:6) test(754, setkey(DT[,c("x1","y1","x2","y2"):=list(x,y)],a), data.table(a=letters[1:3],x=3:1,y=6:4,x1=3:1,y1=6:4,x2=3:1,y2=6:4,key="a")) # And non-recycling i.e. that a single column copy does copy the column diff --git a/src/assign.c b/src/assign.c index 831cbbb92d..cd4c12d3c3 100644 --- a/src/assign.c +++ b/src/assign.c @@ -477,7 +477,9 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v vlen = length(thisvalue); if (length(rows)==0 && targetlen==vlen && (vlen>0 || nrow==0)) { if ( MAYBE_SHARED(thisvalue) || // set() protects the NAMED of atomic vectors from .Call setting arguments to 2 by wrapping with list - (TYPEOF(values)==VECSXP && i>LENGTH(values)-1)) { // recycled RHS would have columns pointing to others, #185. + (TYPEOF(values)==VECSXP && i>LENGTH(values)-1) || // recycled RHS would have columns pointing to others, #185. + (TYPEOF(values)!=VECSXP && i>0) // assigning the same values to a second column. Have to ensure a copy #2540 + ) { if (verbose) { if (length(values)==length(cols)) { // usual branch From ea384e0fd37f36d7af72edc7ad439e1a53529fd3 Mon Sep 17 00:00:00 2001 From: Matt Dowle Date: Wed, 10 Jan 2018 13:44:58 -0800 Subject: [PATCH 2/2] In the new test 753.1 which nicely tests output=, added data correct as well. Added 753.2 too. Checked these tests fail in current dev and CRAN release. --- NEWS.md | 2 +- inst/tests/tests.Rraw | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index dfec507cf5..51d1ca1ea1 100644 --- a/NEWS.md +++ b/NEWS.md @@ -117,7 +117,7 @@ 28. CJ() works with multiple empty vectors now [#2511](https://github.com/Rdatatable/data.table/issues/2511). Thanks to @MarkusBonsch for fixing. -29. `:=` assignment of one vector to two or more columns, e.g. DT[, c("x", "y") := 1:10], caused a tricky error if `setnames` was called afterwards because the code failed to copy the vector. [#2540](https://github.com/Rdatatable/data.table/issues/2540). This is an old issue [#185](https://github.com/Rdatatable/data.table/issues/185) that had been fixed but reappeared when code was refactored. Thanks to @patrickhowerter for the report and the reproducible example and to @MarkusBonsch for fixing. +29. `:=` assignment of one vector to two or more columns, e.g. `DT[, c("x", "y") := 1:10]`, failed to copy the `1:10` data causing errors later if and when those columns were updated by reference, [#2540](https://github.com/Rdatatable/data.table/issues/2540). This is an old issue ([#185](https://github.com/Rdatatable/data.table/issues/185)) that had been fixed but reappeared when code was refactored. Thanks to @patrickhowerter for the detailed report with reproducible example and to @MarkusBonsch for fixing and strengthening tests so it doesn't reappear again. #### NOTES diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index b9f57b8966..f7479ed9f8 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -2151,7 +2151,9 @@ test(752, DT[,head(.SD,2)[,new:=1:.N],by=a], data.table(a=rep(1:3,each=2),b=c(1: # Test duplicate() of recycled plonking RHS, #2298 DT = data.table(a=letters[3:1],x=1:3) test(753, setkey(DT[,c("x1","x2"):=x],a), data.table(a=letters[1:3],x=3:1,x1=3:1,x2=3:1,key="a")) -test(753.1, DT[,c("x1","x2"):=3:1, verbose = TRUE], output = "RHS for item 2 has been duplicated") +test(753.1, DT[,c("x1","x2"):=4:6, verbose = TRUE], data.table(a=letters[1:3],x=3:1,x1=4:6,x2=4:6,key="a"), + output = "RHS for item 2 has been duplicated") +test(753.2, DT[2,x2:=7L], data.table(a=letters[1:3],x=3:1,x1=4:6,x2=c(4L,7L,6L),key="a")) DT = data.table(a=letters[3:1],x=1:3,y=4:6) test(754, setkey(DT[,c("x1","y1","x2","y2"):=list(x,y)],a), data.table(a=letters[1:3],x=3:1,y=6:4,x1=3:1,y1=6:4,x2=3:1,y2=6:4,key="a")) # And non-recycling i.e. that a single column copy does copy the column