[ben-schwen]

Towards #852

This PR is the starting point of modularizing [.

I will build it as stack on top of each other. I moved the tests to a new script since [ is such a central piece that I thought it deserves its own place. Moreover, it gives us the chance to set them as optional.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.09%. Comparing base (7b7f63b) to head (08f3c81).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7931   +/-   ##
=======================================
  Coverage   99.08%   99.09%           
=======================================
  Files          88       88           
  Lines       17420    17420           
=======================================
+ Hits        17261    17262    +1     
+ Misses        159      158    -1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MichaelChirico]

Aside -- are you using gh to manage the stack? Curious how the experience is. I still swear by jj but it doesn't play that nicely with managing the stack on GitHub -- it's trivial to manage the stack locally with jj, but conveying that to GitHub, less so.

[ben-schwen]

Aside -- are you using gh to manage the stack? Curious how the experience is. I still swear by jj but it doesn't play that nicely with managing the stack on GitHub -- it's trivial to manage the stack locally with jj, but conveying that to GitHub, less so.

Nah I'm doing it quite old school. I have a development branch where I do everything and push a lot locally. Before pushing to github I cherry-pick and squash manually.

[MichaelChirico]

well consider this my shameless plug for jj, which I've been using exclusively over git for a while now :)

[tdhock]

you guys don’t use magit ??!! (emacs joke)

[tdhock]

Error on CI is

Error in eval(bysub, xss, parent.frame()) : 
  numeric 'envir' arg not of length one
Calls: <Anonymous> ... system.time -> do.call -> <Anonymous> -> run_bench_mark

To reproduce locally I did

tinfo <- atime::atime_pkg_test_info("~/R/data.table")
tcall <- tinfo$test.call[["DT[i,j,by] per-call overhead for #852"]]
options(datatable.verbose=TRUE)
tres <- eval(tcall)

and I got

i clause present and columns used in by detected, only these subset: [g]
cedta decided 'data.table.7b7f63b04693991e014e245c8d4a0bc6b9dc38c6' wasn't data.table aware. Here is call stack with [[1L]] applied:
[[1]]
eval

[[2]]
eval

[[3]]
atime::atime_versions
…
[[7]]
run_bench_mark

[[8]]
suppressWarnings

[[9]]
withCallingHandlers

[[10]]
eval

[[11]]
eval

[[12]]
bench::mark

[[13]]
eval_one

[[14]]
eval

[[15]]
eval

[[16]]
data.table.7b7f63b04693991e014e245c8d4a0bc6b9dc38c6:::`[.data.table`

[[17]]
`[.data.table`

[[18]]
cedta

Error in eval(bysub, xss, parent.frame()) : 
  numeric 'envir' arg not of length one

This is happening when i is used at same time as by: (the issue does not happen in the other two usages in the test case, because they do not use i with by)

data.table:::`[.data.table`(dt, a > 2L, .(s = sum(v)), by = g)

which calls this code in [.data.table:

            xss = `[.data.table`(x,irows,allbyvars,with=FALSE,nomatch=nomatch,mult=mult,roll=roll,rollends=rollends)
          } else {
            if (verbose) catf("i clause present but columns used in by not detected. Having to subset all columns before evaluating 'by': '%s'\n", deparse(by))
            xss = `[.data.table`(x,irows,nomatch=nomatch,mult=mult,roll=roll,rol

I think the issue is that cedta thinks these recursive calls are not dt-aware? Not sure how to fix.

[ben-schwen]

I think the issue is that cedta thinks these recursive calls are not dt-aware? Not sure how to fix.

Yes. I hope my fix works, fingers crossed :D

[github-actions]

• HEAD=brk/a1_tests much slower for DT[by,verbose=TRUE] improved in #6296
• HEAD=brk/a1_tests faster P<0.001 for memrecycle regression fixed in #5463

(Plot image upload to picrd.com failed; download the artifact below to view plots)

Generated via commit 08f3c81

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	1 minutes and 36 seconds
Installing different package versions	9 minutes and 8 seconds
Running and plotting the test cases	3 minutes and 49 seconds