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.

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

setDT and [,:=] should share code for re-assigning data.tables #6702

Open
MichaelChirico opened this issue Jan 2, 2025 · 2 comments
Open

Comments

@MichaelChirico
Copy link
Member

These two regions are close to identical:

data.table/R/data.table.R

Lines 1224 to 1236 in 70c64ac

} else if (name %iscall% c('$', '[[') && is.name(name[[2L]])) {
k = eval(name[[2L]], parent.frame(), parent.frame())
if (is.list(k)) {
origj = j = if (name[[1L]] == "$") as.character(name[[3L]]) else eval(name[[3L]], parent.frame(), parent.frame())
if (is.character(j)) {
if (length(j)!=1L) stopf("Cannot assign to an under-allocated recursively indexed list -- L[[i]][,:=] syntax is only valid when i is length 1, but its length is %d", length(j))
j = match(j, names(k))
if (is.na(j)) internal_error("item '%s' not found in names of list", origj) # nocov
}
.Call(Csetlistelt,k,as.integer(j), x)
} else if (is.environment(k) && exists(as.character(name[[3L]]), k)) {
assign(as.character(name[[3L]]), x, k, inherits=FALSE)
}

data.table/R/data.table.R

Lines 2970 to 2985 in 70c64ac

} else if (name %iscall% c('$', '[[') && is.name(name[[2L]])) {
# common case is call from 'lapply()'
k = eval(name[[2L]], parent.frame(), parent.frame())
if (is.list(k)) {
origj = j = if (name[[1L]] == "$") as.character(name[[3L]]) else eval(name[[3L]], parent.frame(), parent.frame())
if (length(j) == 1L) {
if (is.character(j)) {
j = match(j, names(k))
if (is.na(j))
stopf("Item '%s' not found in names of input list", origj)
}
}
.Call(Csetlistelt,k,as.integer(j), x)
} else if (is.environment(k) && exists(as.character(name[[3L]]), k)) {
assign(as.character(name[[3L]]), x, k, inherits=FALSE)
}

To keep them in sync, the logic should be extracted to an appropriate helper.

@nipun-gupta-3108
Copy link

Hi @maintainers,

I’m interested in contributing to this issue. I’ve reviewed the functionality of setDT and [:=] and would like to confirm the following before proceeding:

Could you point me to the primary files where setDT and [:=] handle re-assignments?
Are there specific parts of the logic you’d like to see refactored or reused between these two functions?
Should the shared logic be implemented in R, or would a C utility function be more appropriate?
Additionally, do you have specific test cases or benchmarks you’d like to see included in this enhancement?

Thanks for your guidance!

Best regards,
Nipun

@MichaelChirico
Copy link
Member Author

Hi Nipun, if you click through on the code blocks above, it will take you to the place in the source code where you'll find the relevant code for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants