Skip to content

capture the calling scope once as callenv in [ - #7934

Open
ben-schwen wants to merge 1 commit into
brk/a2_missing_flagsfrom
brk/a3_callenv
Open

ben-schwen wants to merge 1 commit into
brk/a2_missing_flagsfrom
brk/a3_callenv

Conversation

@ben-schwen

Copy link
Copy Markdown
Member

Towards #852

Caches calling scope via parent.frame()

@codecov

codecov Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.09%. Comparing base (8e8d36b) to head (74c52e9).

Additional details and impacted files
@@                  Coverage Diff                  @@
##           brk/a2_missing_flags    #7934   +/-   ##
=====================================================
  Coverage                 99.09%   99.09%           
=====================================================
  Files                        88       88           
  Lines                     17430    17431    +1     
=====================================================
+ Hits                      17272    17273    +1     
  Misses                      158      158           

☔ 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.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
  • HEAD=brk/a3_callenv 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 74c52e9

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

Task Duration
R setup and installing dependencies 34 minutes and 40 seconds
Installing different package versions 13 minutes and 54 seconds
Running and plotting the test cases 5 minutes and 55 seconds

Comment thread R/data.table.R
setalloccol(x, n, verbose=verbose) # always assigns to calling scope; i.e. this scope
if (is.name(name)) {
assign(as.character(name),x,parent.frame(),inherits=TRUE)
assign(as.character(name),x,callenv,inherits=TRUE)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

while we're here

Suggested change
assign(as.character(name),x,callenv,inherits=TRUE)
assign(as.character(name), x, callenv, inherits=TRUE)

Comment thread R/data.table.R
assign(as.character(name),x,callenv,inherits=TRUE)
} else if (.is_simple_extraction(name)) {
.reassign_extracted_table(name, x)
.reassign_extracted_table(name, x, callenv)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice! this one's tougher to spot

.reassign_extracted_table = function(name, value, env = parent.frame(2L)) {

Capture parent.frame() once at the top as callenv and use that for every evaluation in the caller's scope (i, j, by, .SDcols, on, :=, ..var, the .N swap, SDenv's parent, ...).

No behaviour change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants