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.
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
Fix #4783 with a designated option #4947
Conversation
This continues the discussion in this PR |
I'm not sure how to continue - all tests pass for me locally. Could it be a merge conflict I missed? |
inst/tests/tests.Rraw
Outdated
d[, a:=c(10, 11)] # not leaking to df | ||
test(2164.2, df$a[1], 1) | ||
d[!is.na(a), b:=c(20,21)] # leaks to df | ||
test(2166, df$b[1], 20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue is test numbering. here is 2166, next is 2164.3 -- tests should be in increasing order in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I just added another commit (with other improvements) and hopefully this is fixed.
…d of a global option which impacts all DT ops.
This PR adds an option that disables in-place assignment - thereby solving #4783 and also indirectly the old #3215.
The option is currently initialized to TRUE (old behaviour) - but at least for us the right initialization is FALSE (fixed behaviour).
Measurements on various size DTs show that the added performance overhead (extra allocations) is definitely manageable.