new with optimization to allow avoid [ overhead

[jangorecki]

closes #3735 and #4485
from 27s down to 5s, together with #4484
and 13s down to 5s for this PR alone

library(data.table)
allIterations <- data.frame(v1 = runif(1e5), v2 = runif(1e5))
DoSomething <- function(row) someCalculation <- row[["v1"]] + 1
system.time(for (r in 1:nrow(allIterations)) DoSomething(allIterations[r, ]))
#   user  system elapsed 
#  3.384   0.007   3.392
allIterations <- as.data.table(allIterations)
setDTthreads(1)
system.time(for (r in 1:nrow(allIterations)) DoSomething(allIterations[r, , with=c(i=FALSE)]))
#   user  system elapsed 
#  5.432   0.121   5.554 

Once the general idea and API will be approved, the following items should be added:

• extra escapes of with and j processing in downstream code in [.data.table
• manual
• more tests
• news

[codecov]

Codecov Report

Merging #4488 into master will decrease coverage by 0.11%.
The diff coverage is 75.71%.

@@            Coverage Diff             @@
##           master    #4488      +/-   ##
==========================================
- Coverage   99.60%   99.48%   -0.12%     
==========================================
  Files          72       73       +1     
  Lines       13918    13988      +70     
==========================================
+ Hits        13863    13916      +53     
- Misses         55       72      +17     

Impacted Files	Coverage Δ
R/with.R	74.50% <74.50%> (ø)
R/data.table.R	99.78% <78.94%> (-0.22%)	⬇️

---

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 d47a83f...45952d1. Read the comment docs.

[MichaelChirico]

a bit hard to figure out what this branch is for

[jangorecki]

from body of this branch one can see that branch is for early return and escape rest of [ processing

[MichaelChirico]

basic API looks good. Do you know if with=FALSE now is faster than an equivalent with=TRUE query? I worry if we build this as "faster row selectionandwith=FALSE` is not "faster column selection" it would be confusing

[jangorecki]

Do you know if with=FALSE now is faster than an equivalent with=TRUE query?

if it is identical(with, FALSE) then new optimization is totally escaped, to stay backward compatible, so is not faster. For with=c(j=FALSE) it will be faster, but won't be replacement of with=FALSE because it will not handle !"a" or a:b. It will be really non-NSE.

I worry if we build this as "faster row selection" and with=FALSE` is not "faster column selection" it would be confusing.

The "faster" is not that much relevant here, I would call it low overhead, which eventually can be faster when looping many times. Otherwise speed difference is insignificant.

[MichaelChirico]

OK. Still if "low-overhead-ness" differs between with=FALSE and with = c(i=FALSE), it may be confusing.

[jangorecki]

with=FALSE is not really low-overhead, with=c(j=FALSE) is. Agree it deserves good documentation. The good thing is that change is fully backward compatible, so use of with=FALSE won't be affected.

[ColeMiller1]

Most options are lists, including discussion of nomatch = NULL discusses the use of a list. For consistency, I wonder if with = .(i = TRUE, j = FALSE) would be more data.tableish.

For some parts, maybe we could include `.` = function(...) list(...) so we do not have to do NSE.

[jangorecki]

@ColeMiller1 to make this NSE (so the argument can be evaluated in separate line and result passed to with) we would need to export . which is not a good idea. Such "consistency" doesn't make much sense, if we go that way we will ended up using allow.cartesian=.(TRUE) as well. Let's use that when it can solve a new problem or overcome existing limitation.
Eventually accepting with=list(i=FALSE, j=FALSE) could make sense to more explicitly express difference vs non-optimized with interface.

[ColeMiller1]

This seq_len(nr) is only needed on the which logic branch. Otherwise, we could do i = NULL

[jangorecki]

Yes, but eventually subsetDT could not copy when NULL provided, although it does copy now.

[ColeMiller1]

The main point being that if missing(i), this would realize an integer vector seq_len(nr) when it is unnecessary except for the which() branch.

Regarding allowing shallow copies in CsubsetDT, that would be a nice feature. A data.frame does not appear to make a copy when selecting columns. At least that's what memory profiling using bench::mark suggests.