Repository navigation
move helper functions outside of [ - #7935
ben-schwen wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## bracket_modularization #7935 +/- ##
=========================================================
Coverage ? 99.09%
=========================================================
Files ? 88
Lines ? 17430
Branches ? 0
=========================================================
Hits ? 17272
Misses ? 158
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
94d7529 to
4821512
Compare
4821512 to
296435e
Compare
(Plot image upload to picrd.com failed; download the artifact below to view plots) Generated via commit ca6af09 Download link for the artifact containing the test results: ↓ atime-results.zip
|
296435e to
b7fe976
Compare
| jvnames[head(key_idx, missing_keys[1L] - 1L)] | ||
| } | ||
| shared_keys = get_shared_keys(jsub, jvnames, sdvars = sdvars, key(x)) | ||
| shared_keys = .dt_get_shared_keys(jsub, jvnames, sdvars = sdvars, key(x)) |
There was a problem hiding this comment.
| shared_keys = .dt_get_shared_keys(jsub, jvnames, sdvars = sdvars, key(x)) | |
| shared_keys = .dt_get_shared_keys(jsub, jvnames, sdvars, key(x)) |
| raise_condition = function(signal, message, classes, immediate=FALSE, appendLF=FALSE) { | ||
| obj = list(message=message, call=sys.call(sys.nframe()-2L)) | ||
| n = sys.nframe() - 2L # the caller of stopf() etc. | ||
| # helpers extracted from [.data.table are named .dt_*; their conditions report the call of [.data.table (or whoever called them) |
There was a problem hiding this comment.
Maybe they should be named .dtq_ (query) to distinguish them from other internal functions?
Another alternative to consider (especially w.r.t. potential performance drawbacks) would be to S3 them:
.zip_items = function(...)
class(.zip_items) = "dt_query_helper"And here:
while (inherits(sys.call(n), "dt_query_helper")) ...(I haven't tested if sys.call() retains the attribute).
At a higher level, is it really that much more beneficial to see "Error in [.data.table" than "Error in .zip_items"? The latter would appear to be more specific about which of the many steps in [ went awry; and beginner users can be "scared" by "[.data.table" too (they've never written [.data.table before -- where did it come from?). Would just using call.=FALSE suffice?
Also, be sure to evaluate the robustness of the proposed solution. I have memories of endless headaches trying to inspect the call stack and provide better errors in {bit64} where I wanted to know "Did a user call this exported S3 method directly, or did they get here from the generic?". I ultimately decided it was impossible to resolve in full generality. Edge cases to consider: DT() where [ gets masked by another name; tryCatch() / withCallingHandlers(); substitute().
There was a problem hiding this comment.
library(bench)
by_name = function(n) { call = sys.call(n); is.call(call) && is.name(f <- call[[1L]]) && startsWith(as.character(f), ".dt_") }
by_class = function(n) inherits(sys.function(n), "dt_query_helper")
walk = function(check) {
n = sys.nframe() - 1L
parents = sys.parents()
while (n > 0L && parents[n] > 0L && check(n)) n = parents[n]
n
}
.dt_inner = function(check) walk(check)
.dt_outer = function(check) .dt_inner(check)
class(.dt_inner) = class(.dt_outer) = "dt_query_helper"
`[.fake` = function(x, check) .dt_outer(check)
x = structure(list(), class="fake")
stopifnot(identical(x[by_name], x[by_class]))
mark(by_name = x[by_name], by_class = x[by_class], iterations=1e5)
# A tibble: 2 × 13
# expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result memory time gc
# <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> <list> <list> <list>
# 1 by_name 3.25µs 4.01µs 233464. 0B 133. 99943 57 428ms <int [1]> <Rprofmem [0 × 3]> <bench_tm [100,000]> <tibble [100,000 × 3]>
# 2 by_class 2.29µs 2.83µs 324089. 0B 149. 99954 46 308ms <int [1]> <Rprofmem [0 × 3]> <bench_tm [100,000]> <tibble [100,000 × 3]>so should be same in terms of speed :)
b7fe976 to
efa5d39
Compare
efa5d39 to
f1ecec5
Compare
f1ecec5 to
671b164
Compare
671b164 to
5cced2e
Compare
5cced2e to
3c4d87f
Compare
Functions defined inside [ capture its frame, so they block moving the code that uses them. Move them to internal top-level functions with class "dt_query_helper", so that conditions they raise still report the [.data.table call. No behaviour change.
3ca2c81 to
ca6af09
Compare
Towards #852
Since this change consists of a lot code moving I would recommend reviewing it with
git diff brk/a4_move_helpers^! --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-changewhich displays the real changes nicely