Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for .datatable.aware being FALSE #5655

Merged
merged 16 commits into from
Feb 19, 2024
Merged

Check for .datatable.aware being FALSE #5655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9330709
Check for .datatable.aware being FALSE #5654
dvg-p4 Jun 12, 2023
0bebdf5
Add tests
dvg-p4 Jan 15, 2024
1e96158
Fix tests
dvg-p4 Jan 15, 2024
daaeb41
Simplify logic as suggested
dvg-p4 Jan 15, 2024
f86892a
Band-aid on underlying selfrefok() problem for test
dvg-p4 Jan 15, 2024
0ae4919
Update news and add comment
dvg-p4 Jan 15, 2024
75601dd
Fix transform slowness (#5493)
OfekShilon Jan 6, 2024
d7baaf8
Improvements to the introductory vignette (#5836)
Anirban166 Jan 6, 2024
44a5adc
switch to 3.2.0 R dep (#5905)
MichaelChirico Jan 12, 2024
a47b195
frollmax1: frollmax, frollmax adaptive, left adaptive support (#5889)
jangorecki Jan 12, 2024
274a59d
Run GHA jobs on 1-15-99 dev branch (#5909)
MichaelChirico Jan 14, 2024
8eaa5a1
Make declarations static for covr (#5910)
MichaelChirico Jan 15, 2024
ec90e7c
botched rebase
MichaelChirico Feb 19, 2024
618e01c
stray \
MichaelChirico Feb 19, 2024
d591431
smaller diff
MichaelChirico Feb 19, 2024
476049e
test #s
MichaelChirico Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
# 2:
```

2. `cedta()` now returns `FALSE` if `.datatable.aware = FALSE` is set in the calling environment, [#5654](https://github.com/Rdatatable/data.table/issues/5654).

## NOTES

1. `transform` method for data.table sped up substantially when creating new columns on large tables. Thanks to @OfekShilon for the report and PR. The implemented solution was proposed by @ColeMiller1.
Expand Down
8 changes: 6 additions & 2 deletions R/cedta.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ cedta.pkgEvalsUserCode = c("gWidgetsWWW","statET","FastRWeb","slidify","rmarkdow
# package authors could set it using assignInNamespace and then not revert its value properly which would
# cause subsequent calls from other packages to fail.

# .datatable.aware = TRUE or .datatable.aware = FALSE can also be set directly in the calling frame,
# e.g. at the start of a particular function to make it data.table-aware in an otherwise unaware package
# or vice versa.

# nocov start: very hard to reach this within our test suite -- the call stack within a call generated by e.g. knitr
# for loop, not any(vapply_1b(.)), to allow early exit
.any_eval_calls_in_stack <- function() {
Expand All @@ -39,8 +43,8 @@ cedta.pkgEvalsUserCode = c("gWidgetsWWW","statET","FastRWeb","slidify","rmarkdow
cedta = function(n=2L) {
# Calling Environment Data Table Aware
env = parent.frame(n)
if (isTRUE(env$.datatable.aware)) { # dtplyr 184
return(TRUE)
if (isTRUEorFALSE(env$.datatable.aware)) { # dtplyr#184, #5654
return(env$.datatable.aware)
}
ns = topenv(env)
if (!isNamespace(ns)) {
Expand Down
10 changes: 10 additions & 0 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18268,3 +18268,13 @@ test(2244.2, DT[, let(a=1, )], error="let.*Did you forget a trailing comma\\?")
test(2244.3, DT[, `:=`(a=1, , b=2)], error="`:=`.*\\[2\\]")
test(2244.4, DT[, let(a=1, , b=2)], error="let.*\\[2\\]")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if selfrefok() becomes FALSE after returning from cedta(), isn't that a broader issue?

Now users who declare themselves unaware of data.table are getting obscure internal warnings from data.table....

WDYT? Worth addressing in this PR? Or kick the can until a further issue is reported downstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it actually propagates to a user-visible warning in normal use--it's specifically data.table::test() that checks selfrefok() and throws an error if not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's wait & see then.

test(2244.5, DT[, `:=`(a=1, , b=2, )], error="[2, 4]")

# allow specifying datatable-unaware behavior, #5654 / #5655
dt = data.table(foo = 1:3, bar = 4:6)
.datatable.aware = TRUE
test(2245.1, dt[1], data.table(foo = 1L, bar = 4L)) # Single index should be interpreted as row
.datatable.aware = FALSE
# selfrefok() fails on unaware-indexed datatables; the copy() here sidesteps that issue.
test(2245.2, copy(dt[1]), data.table(foo = 1:3)) # Revert to data.frame syntax: Interpret index as column
rm(.datatable.aware)
test(2245.3, dt[1], data.table(foo = 1L, bar = 4L)) # Default in this environment should be datatable-aware
Loading