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

names<- retains sorting attributes #5849

Merged
merged 3 commits into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ S3method(dim, data.table)
S3method(dimnames, data.table)
S3method("dimnames<-", data.table)
S3method("names<-", data.table)
S3method("colnames<-", data.table)
S3method(duplicated, data.table)
S3method(unique, data.table)
S3method(merge, data.table)
Expand Down
8 changes: 5 additions & 3 deletions R/data.table.R
Original file line number Diff line number Diff line change
Expand Up @@ -2250,15 +2250,17 @@ dimnames.data.table = function(x) {
x # this returned value is now shallow copied by R 3.1.0 via *tmp*. A very welcome change.
}

"names<-.data.table" = function(x,value)
"names<-.data.table" = "colnames<-.data.table" = function(x,value)
{
# When non data.table aware packages change names, we'd like to maintain the key.
# If call is names(DT)[2]="newname", R will call this names<-.data.table function (notice no i) with 'value' already prepared to be same length as ncol
x = shallow(x) # `names<-` should not modify by reference. Related to #1015, #476 and #825. Needed for R v3.1.0+. TO DO: revisit
x = .shallow(x, retain.key=TRUE) # `names<-` should not modify by reference. Related to #1015, #476 and #825. Needed for R v3.1.0+. TO DO: revisit
if (is.null(value))
setattr(x,"names",NULL) # e.g. plyr::melt() calls base::unname()
else
else {
setnames(x,value)
setalloccol(x)
}
x # this returned value is now shallow copied by R 3.1.0 via *tmp*. A very welcome change.
}

Expand Down
32 changes: 32 additions & 0 deletions inst/tests/tests.Rraw
Original file line number Diff line number Diff line change
Expand Up @@ -18111,3 +18111,35 @@ test(2238.9, NA %notin% c(1:5, NA), FALSE)

# shift actionable error on matrix input #5287
test(2239.1, shift(matrix(1:10, ncol = 1)), error="consider wrapping")

# names<- retains index and key, #5125, #5126, #5126, #5128
Copy link
Member

Choose a reason for hiding this comment

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

Great work.
Could we add rest of unit tests added in #5133?
If some of them fails then possibly comment them out with explanation. So we have clarity what from #5133 is being covered.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea. I skipped originally bc I wasn't sure whether output change tests are worth copying.

I'll read more carefully and copy over the worthwhile tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, copied over the tests. They caught two things:

  1. colnames(x) <- is now treated the same a names(x) <-. Requires a new S3 method registration.
  2. Doing names(x) <- "..."; set(x, j=j, val=val) is fixed with a simple setalloccol(). Maybe not ideal, but a good expedient.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any tests being commented out. Does it mean we cover all tests from Matt's PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

DT <- data.table(a = 1, b = 2)
setkey(DT, a)
setindex(DT, b)
names(DT) <- c("c", "d")
test(2240.1, key(DT), "c")
test(2240.2, indices(DT), "d")

# Test warnings for names<- and colnames<-, but only warnings when caller is data.table aware.
DT = data.table(a=1:3, b=4:6, key="a")
test(2241.01, names(DT)[1]<-"A", "A")
test(2241.02, DT, data.table(A=1:3, b=4:6, key="A")) # key wasn't retained in dev after #5084
test(2241.03, DT[, C:=1], data.table(A=1:3, b=4:6, C=1, key="A")) # ensure over-allocated ok and no warning
test(2241.04, set(DT, j="D", value=2), data.table(A=1:3, b=4:6, C=1, D=2, key="A")) # using set() too
test(2241.05, colnames(DT)[2]<-"B", "B")
test(2241.06, DT, data.table(A=1:3, B=4:6, C=1, D=2, key="A"))
test(2241.07, set(DT, j="E", value=3), data.table(A=1:3, B=4:6, C=1, D=2, E=3, key="A"))
test(2241.08, names(DT)<-letters[1:5], letters[1:5]) # R doesn't copy *tmp* when assigning to all names
test(2241.09, DT, data.table(a=1:3, b=4:6, c=1, d=2, e=3, key="a"))
test(2241.10, set(DT, j="f", value=4), data.table(a=1:3, b=4:6, c=1, d=2, e=3, f=4, key="a"))

# spotted by @ColeMiller1 in https://github.com/Rdatatable/data.table/pull/5133/files#r1320780851
DT = data.table(id=1:2, x=1:2)
r = copy(DT)[, x := 5L]
test(2241.11, DT, data.table(id=1:2, x=1:2))
test(2241.12, r, data.table(id=1:2, x=c(5L,5L)))

DT = data.table(id=1:2, x=1:2)
r = copy(DT)[1L, x:= 5L]
test(2241.13, DT, data.table(id=1:2, x=1:2))
test(2241.14, r, data.table(id=1:2, x=c(5L,2L)))
Loading