Should never recycle the zero-length vector

[shrektan]

Closes #3727

[codecov]

Codecov Report

Merging #4262 (15514b0) into master (7fbb471) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4262   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files          73       73           
  Lines       14446    14447    +1     
=======================================
+ Hits        14363    14364    +1     
  Misses         83       83           

Impacted Files	Coverage Δ
R/as.data.table.R	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fbb471...15514b0. Read the comment docs.

[shrektan]

@mattdowle It would be great if this patch can get your approval first, as I'm developping an internal package where DT[J('a', x), ...] is used a lot while x could possible be a zero-length date... The current behavior forces me to handle the length-0 x case specially, literally everywhere... If you agree with this patch's behavior, I can ensure I'm good to depend on this patch, which will save my lots of efforts...

Thanks a lot.

[jangorecki]

1967.35 and 2139.1 are currently failing in this PR, it is quite likely that some revdeps might be failing as well, blocking release process. IF the change is going to be incorporated, it needs to be made in a less breaking manner. Maybe hold on with farther changes till we all agree on behavior.

---

I am having the very same problem in #4370. I need to recycle there as well, but my preference is to recycle 0 and 1 rows data.table to bigger one, quite opposite what you are trying to push here.
Good references for you can be data.frame function, and for me cbind, they both seems to be consistent on recycling, 0 rows throws error.

d1 = 1L
d2 = 1:2

data.frame(d2, d1)
#  d2 d1
#1  1  1
#2  2  1
cbind(d2, d1)
#     d2 d1
#[1,]  1  1
#[2,]  2  1

[shrektan]

The problem is:

Is recyling the zero-length element meaningful, since there's no value at all? Reclying it with NA is not necessarily a reasonable one.

That's why data.frame, cbind throws error while tibble() gives the zero-row table.

In addition, as I've mentioned in #3727, recycling the zero-length element will trigger a warning of data.table. Someone may accidentally depend this behavior, but I doubt anyone would do it intentionally... Anyway, I will take an investigation on the failed tests.

[jangorecki]

Agree, you have my vote for recycling to 0 rows. It is easy to escape that by DT[J('a', x[1L]), ...], the other way around is not that easy.

[MichaelChirico]

LGTM

[jangorecki]

This email seems slightly related: https://stat.ethz.ch/pipermail/r-devel/2020-May/079572.html

[mattdowle]

Your examples really helped, thanks.

[mattdowle]

I reran all revdeps to check the impact. After resolving unrelated issues, only one possible impact (RstoxData) which is comforting. Even that one might be something else in dev and not this change. I'll file a separate issue.

> status()
Installed data.table to be tested against: 1.14.1 2021-04-28 04:39:30
CRAN:
 ERROR   :    2 : RstoxData traitdataform 
 WARNING :    1 : fwildclusterboot 
 NOTE    :  350 
 OK      :  698 
 TOTAL   : 1051 / 1051

> cran()
tools::CRAN_check_results() returned 226,625 rows in 5.584s elapsed (1.249s cpu) 
R-release is used for revdep checking so comparing to CRAN results for R-release
            Package r-release-linux-x86_64 r-release-macos-x86_64 r-release-windows-ix86+x86_64
1:        RstoxData                     OK                   NOTE                            OK
2:    traitdataform                  ERROR                     OK                         ERROR
3: fwildclusterboot                   WARN                     OK                          WARN