Codecov Report

Merging #5501 (bb08e14) into master (19b7866) will decrease coverage by 1.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5501      +/-   ##
==========================================
- Coverage   99.51%   98.32%   -1.19%     
==========================================
  Files          80       80              
  Lines       14774    14801      +27     
==========================================
- Hits        14702    14553     -149     
- Misses         72      248     +176     

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

could you add a test where this matters? probably worth a NEWS item as well, this could potentially break some folks

could you add a test where this matters? probably worth a NEWS item as well, this could potentially break some folks

I'm not aware of any case where this change makes an observable difference in behavior. The best I can suggest is a test documenting the performance improvement in an awkward call of the sort transform used to make:

t1 <- system.time(dt1 <- data.table(a=runif(1e5), b=runif(1e5)))
t2 <- system.time(dt2 <- do.call(data.table, list(dt1)))

test(2239.1, all.equal(dt1,dt2), TRUE)
test(2239.2, t2["elapsed"]/t1["elapsed"]<0.5, TRUE)   # On my system this is >30 before the fix, <0.2 after

Do you think this adds value?

It matters here:

asodijfoaisjdfoiajsdofijasodifjoaisdjfoiajsdoifjaosidjfoiajsdofijaosidjfoiasjdofijasdoifjaosidjfoiasjdfiuajsfuasiudfhoiauhjzxclkvjbhlszkjxhcliuasdhfgoiuahsdoufhlijxchlkzjhvkjhaljshdfiluhcubvjdfbvjkahsdfliuhawpioedhflaijsdh = function() 1L
data.table(c(
 
 asodijfoaisjdfoiajsdofijasodifjoaisdjfoiajsdoifjaosidjfoiajsdofijaosidjfoiasjdofijasdoifjaosidjfoiasjdfiuajsfuasiudfhoiauhjzxclkvjbhlszkjxhcliuasdhfgoiuahsdoufhlijxchlkzjhvkjhaljshdfiluhcubvjdfbvjkahsdfliuhawpioedhflaijsdh(),
 
 asodijfoaisjdfoiajsdofijasodifjoaisdjfoiajsdoifjaosidjfoiajsdofijaosidjfoiasjdofijasdoifjaosidjfoiasjdfiuajsfuasiudfhoiauhjzxclkvjbhlszkjxhcliuasdhfgoiuahsdoufhlijxchlkzjhvkjhaljshdfiluhcubvjdfbvjkahsdfliuhawpioedhflaijsdh(),
 
 asodijfoaisjdfoiajsdofijasodifjoaisdjfoiajsdoifjaosidjfoiajsdofijaosidjfoiasjdofijasdoifjaosidjfoiasjdfiuajsfuasiudfhoiauhjzxclkvjbhlszkjxhcliuasdhfgoiuahsdoufhlijxchlkzjhvkjhaljshdfiluhcubvjdfbvjkahsdfliuhawpioedhflaijsdh(),
 
 asodijfoaisjdfoiajsdofijasodifjoaisdjfoiajsdoifjaosidjfoiajsdofijaosidjfoiasjdofijasdoifjaosidjfoiasjdfiuajsfuasiudfhoiauhjzxclkvjbhlszkjxhcliuasdhfgoiuahsdoufhlijxchlkzjhvkjhaljshdfiluhcubvjdfbvjkahsdfliuhawpioedhflaijsdh()
))

There should be a more minimal version of this but hopefully illustrates the point

Do you think this adds value?

We shy away from timing tests in our main suite -- you never know when some system interference / sun spots can cause things to go haywire and now we have to deal with the threat of CRAN removal...

OTOH, there is this non-CRAN suite, couldn't hurt to throw a test like that in there:

https://github.com/Rdatatable/data.table/blob/master/inst/tests/benchmark.Rraw

I'd suggest a threshold of maybe 2 or 5 to be conservative, too, .2 vs .5 seems to tight and likely not to replicate across platforms / with high statistical certainty

There should be a more minimal version of this but hopefully illustrates the point

Nice! deparse has a default width.cutoff = 60L, so these can be trimmed to 61-char names.
I'll do that - and add a conservative timing test to benchmark.Rraw - tomorrow. Thanks.

@MichaelChirico I checked now and think your example also doesn't have an observable impact on behavior. With and without nlines=1, the generated name of the column is "V1". Notice the line after the deparse:

      tmp = if (syms[i]) as.character(dot_sub[[i]]) else deparse(dot_sub[[i]])[1L]
      if (tmp == make.names(tmp)) vnames[i]=tmp

make.names converts newlines, brackets and others to dots, and so does change pretty much anything that slips into this code path - except proper variable names - and causes the deparse to go unused. With and without nlines=1, including your example and anything I could come up with.
This is kind of good news - I probably won't be able to add a test, but the chances of breaking anyone are virtually zero.

I am adding the news item and benchmark test.