-
Notifications
You must be signed in to change notification settings - Fork 1k
feat : added condition to check the RHS of := #7161
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
base: master
Are you sure you want to change the base?
Conversation
Generated via commit c8cec80 Download link for the artifact containing the test results: ↓ atime-results.zip
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7161 +/- ##
=======================================
Coverage 98.50% 98.51%
=======================================
Files 81 81
Lines 15032 15036 +4
=======================================
+ Hits 14808 14812 +4
Misses 224 224 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Why did you open a new PR? https://github.com/Rdatatable/data.table/pull/6742/files has the same changes, right? Can you please close one or the other? |
i closed the previous one ,i thought to make a branch in this repo so ... |
@@ -1411,7 +1411,13 @@ replace_dot_alias = function(e) { | |||
} | |||
|
|||
if (!is.null(lhs)) { | |||
# TODO?: use set() here now that it can add new columns. Then remove newnames and alloc logic above. | |||
newnames = setdiff(lhs, names(x)) |
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.
What's the difference between this assignment and the one above?
Line 1218 in 2f0d12f
newnames=setdiff(lhs, names_x) |
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.
both compute setdiff(lhs, names(x)), but the one above uses the cached names_x. I’ll update the later one to use names_x as well for consistency and to avoid unnecessary recomputation.
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.
sorry i don't understand, why recalculate newnames
at all if it's just the same value as above?
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.
Got it, thanks. The second newnames assignment was redundant. I will take care of it.
@@ -1411,7 +1411,13 @@ replace_dot_alias = function(e) { | |||
} | |||
|
|||
if (!is.null(lhs)) { | |||
# TODO?: use set() here now that it can add new columns. Then remove newnames and alloc logic above. | |||
newnames = setdiff(lhs, names(x)) | |||
if (length(newnames) > 0) { |
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.
does this error not happen for the case of overwriting an existing column?
# TODO?: use set() here now that it can add new columns. Then remove newnames and alloc logic above. | ||
newnames = setdiff(lhs, names(x)) | ||
if (length(newnames) > 0) { | ||
if (is.function(jval)) { |
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.
I don't think this will catch the following case (which you should add as a new regression test):
DT=data.table(a=1:10)
DT[,c('a', 'b'):=.(1:10, mean)]
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.
Overall I suspect it will be more natural to put the fix here inside C
Okay, that makes sense . I’ll try to implement it So far, I’m aware to handle these cases:
Are there any other specific cases or edge conditions I should be aware of? Let me know so I can cover them in the implementation. |
I'm not sure how this differs from the first case. You might try triggering this error from |
Closes #5829
changes made-
At present, this only covers the function case. Other exotic RHS types (e.g., environments, external pointers) are not yet handled.
Is there anything else that should be checked or added to this condition?
i closed the previous pr and opened this, the whole summary is provided above.
hi @MichaelChirico @tdhock can you please review this when you have time
thanks for your time.