Added shallow search for data.table in tables()#7580
Added shallow search for data.table in tables()#7580
Conversation
…o feat/adding_list_search_to_tables
|
Hello, I created a new PR in replacement of #7568 Reasons: There was some git issue there and the merge became too complex and I changed the algo because I didnt know previously that rbind or cbind would cost for re-allocation The current PR considers that part and avoids appends Previous PR : creating seperate data.table called info and rbind at the end |
|
In reply to previous comment of @jangorecki An example of when this new feature could be useful? To support lists which occur due to split.data.table or fread like the following The original code supported data.table() top level and this code adds support for list(data.table) if the arg |
|
Example of the original code and the new feature is as follows
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7580 +/- ##
=======================================
Coverage 99.02% 99.03%
=======================================
Files 87 87
Lines 16896 16937 +41
=======================================
+ Hits 16732 16773 +41
Misses 164 164 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Generated via commit 866820a Download link for the artifact containing the test results: ↓ atime-results.zip
|
inst/tests/tests.Rraw
Outdated
| xenv2$N = list(a = 1:5) | ||
| setkey(xenv2$M$b, a) | ||
| setindex(xenv2$M$b, b) | ||
| test(2360.1, tables(env = xenv2, shallow_search = TRUE)$NAME, c("DT", "L[[1]]", "L[[2]]", "M$b")) |
There was a problem hiding this comment.
prefer saving the output to re-running tables(env = xenv2, shallow_search = TRUE) many times.
alternatively, just do one test like
test(2360.1, tables(env = xenv2, shallow_search = TRUE)[, .(NAME, NROW, NCOL)], data.table(...))| xenv2$M = list(b = data.table(a = 1, b = 4:6), a = 1:5) | ||
| xenv2$N = list(a = 1:5) | ||
| setkey(xenv2$M$b, a) | ||
| setindex(xenv2$M$b, b) |
There was a problem hiding this comment.
nit: I would move setindex() closer to the index=TRUE tests
R/tables.R
Outdated
| } | ||
| } | ||
| else { | ||
| # the original code path when shallow_search=FALSE |
There was a problem hiding this comment.
this comment doesn't make sense outside the context of this PR, "the original code path" will be a relic within a few months
R/tables.R
Outdated
| tables = function(mb=type_size, order.col="NAME", width=80L, | ||
| env=parent.frame(), silent=FALSE, index=FALSE) | ||
| env=parent.frame(), silent=FALSE, index=FALSE, | ||
| shallow_search=FALSE) |
There was a problem hiding this comment.
I am thinking the best way to go about this is actually something like either depth=0 or recursive=FALSE. Then both cases share the same logic, except that the default cuts out after the shallow search.
If the code to do a recursive walk is proving intimidating, we can do depth=0, and this PR can support depth=1 and error for depth>1 "not yet supported" and leave it for future work.
WDYT?
There was a problem hiding this comment.
yeah we can do a depth = 0, 1 and error at depth >1 for this PR
…o feat/adding_list_search_to_tables

Closes #2606
added arg
depth = 1Lto tables() one for shallow searchif
depthis 0 then its the data.tableif
depthis 1, we loop through list-like objects using is.list and which are not data.table or data.frameif
depth> 1, we throw erroradded name for the nested list found
parent[[1]]orparent$childpre-allocating info to avoid reallocation cost