:=(...) with shift() - gforce follow-up #5245, #5348

[ben-schwen]

Follow up to #5245, #5348 which implemented #1414
Closes #5403

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1be897) 97.46% compared to head (9ca25ab) 97.47%.
Report is 20 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5404   +/-   ##
=======================================
  Coverage   97.46%   97.47%           
=======================================
  Files          80       80           
  Lines       14822    14831    +9     
=======================================
+ Hits        14447    14456    +9     
  Misses        375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jangorecki]

merging this PR could potentially create issues in revdeps therefore revdeps check should be repeated after merge

[tdhock]

"revdeps check should be repeated after merge" -> revdeps check is done every evening for master branch. https://github.com/Rdatatable/data.table/wiki/Release-management-and-revdep-checks

[MichaelChirico]

I think we need more tests. What happens in the more ambiguous cases? e.g.

DT <- data.table(a = 1:4, b = 5:8)
DT[, c('out1', 'out2') := shift(list(a, b), n = c(0, 0))]

shift() produces 2x2=4-item output in this case, is it valid to supply length-2 LHS to :=?

Perhaps more definitively, for:

DT[, (...) := shift(list(a, b), n = -1:1)

What are valid lengths of LHS? 1, 2, 3, 6? Or only some of those.

It looks like this is maybe a more fundamental issue with shift() which forces a dimensional collapse across x and n. Should we add an argument simplify (TRUE by default for back-compatibility) which when FALSE guarantees "unnested" output?

[ben-schwen]

I think we need more tests. What happens in the more ambiguous cases? e.g.

DT <- data.table(a = 1:4, b = 5:8)
DT[, c('out1', 'out2') := shift(list(a, b), n = c(0, 0))]

shift() produces 2x2=4-item output in this case, is it valid to supply length-2 LHS to :=?

Nope, it's not valid, but also never has been and I consider that actually a quite good safeguard.

f = shift # to escape GForce
DT[, c('out1', 'out2') := f(list(a, b), n = c(0, 0))]
# Error in `[.data.table`(DT, , `:=`(c("out1", "out2"), f(list(a, b), n = c(0,  : 
#   Supplied 2 columns to be assigned 4 items. Please see NEWS for v1.12.2.
DT[, c('out1', 'out2') := shift(list(a, b), n = c(0, 0))]
# Error in `[.data.table`(DT, , `:=`(c("out1", "out2"), shift(list(a, b),  : 
#   Supplied 2 columns to be assigned 4 items. Please see NEWS for v1.12.2.

[MichaelChirico]

Nope, it's not valid, but also never has been and I consider that actually a quite good safeguard.

Do we have regression tests for this?

[ben-schwen]

Do we have regression tests for this?

There was one for supplying more columns than items. Now we also have the opposite direction.

[jangorecki]

This looks to be ready to merge, is that correct?

[ben-schwen]

Yes.