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 generates shallow copy earlier to avoid interfering with attributes of co-bound tables #6470

Closed
wants to merge 1 commit into from

Conversation

OfekShilon
Copy link
Contributor

Fix #4784.

This replaces the old PR #4901 that was never merged. The discussion/reviews there still apply.

@MichaelChirico MichaelChirico added this to the 1.17.0 milestone Sep 5, 2024
Comment on lines +19068 to +19071
d1 <- data.frame(a=1)
d2 <- d1
setDT(d2)
test(2283, !is.data.table(d1))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
d1 <- data.frame(a=1)
d2 <- d1
setDT(d2)
test(2283, !is.data.table(d1))
d1 = data.frame(a=1)
d2 = d1
setDT(d2)
test(2283, !is.data.table(d1))

data.table style guide to use = instead of <-

@ben-schwen
Copy link
Member

Ty for the PR. Change to looks good!

To cater also for the other corner cases of rownames and colnames I would move setalloccol up.
Also I would add these additional test cases:

d1 = data.frame(a=1:2)
rownames(d1) = c("2", "1")
d2 = d1
setDT(d2)
test(2283.2, rownames(d1), c("1", "2"))
d1 = data.frame(a=1, b=2)
colnames(d1) = c("a", "a")
d2 = d1
setDT(d2, check.names=TRUE)
test(2283.3, colnames(d1), c("a", "a"))

@MichaelChirico MichaelChirico changed the title setDT in a function no longer modifies the class of a data.frame argument setDT generates shallow copy earlier to avoid interfering with attributes of co-bound tables Sep 29, 2024
@MichaelChirico
Copy link
Member

Duplicate of #6551 (now merged)

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.

setDT modifies the class of the origin data.frame
3 participants