merge can't handle custom classes

[ladida771]

The result of A and B merge inherits B's custom classes which does not seem intuitive to me at all.

Example:

A=data.table(x=c(1,2,3),y=c(4,5,6))
B=data.table(x=c(1),w=c(5))
class(B) <- c("custom","data.table","data.frame")

C=merge(A,B,by=c('x'))
class(C)

Expecting: "data.table" "data.frame"
Getting: "custom" "data.table" "data.frame"

[michaelquinn32]

Reopening this.

The case where the result inherits the class of B is clearly wrong. Thanks for fixing that!

But dropping the class of A is also seems incorrect to me. I think about it in terms of other OO languages. Methods usually don't change the class of the object they are called on, and I think it's a good practice to have S3 dispatch maintain the class of the object whose method is used.

IMO, this is the correct behavior:

A <- data.table(x = c(1, 2, 3), y = c(4, 5, 6))
B <- data.table(x = c(1), w = c(5))
class(A) <- c("custom", "data.table", "data.frame")
C <- merge(A, B, by = "x")
class(C)
#> "custom", "data.table", "data.frame"

Granted, merge.data.frame is breaking this rule as well, but that's a much harder thing to fix.

[jangorecki]

@mattdowle not sure if we want this for 1.12.0 because it could eventually break some packages and we are already after revdeps check. I would postponed that to 1.12.2, unless there is going to be another round of revdeps checks before 1.12.0 release