[SPARK-13327][SPARKR] Added parameter validations for colnames<-

[olarayej]

No description provided.

[SparkQA]

Test build #51373 has finished for PR 11220 at commit e8c6415.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[sun-rui]

I would prefer keeping the current signature instead of checking the class of the parameter inside the method.
@felixcheung, what's your opinion?

[felixcheung]

I agree - letting R do type matching for the method signature seems like a better approach?

[olarayej]

The reason I added that check was because of this:

colnames(irisDF) <- 1
Error in colnames<-(*tmp*, value = 1) :
'dimnames' applied to non-array

After I ran that, I saw this:

showMethods("colnames<-")
Function: colnames<- (package SparkR)
x="ANY", value="ANY"
x="DataFrame", value="character"
x="DataFrame", value="numeric"
(inherited from: x="ANY", value="ANY")

So looks like R automatically adds definitions of colnames<- if value is other than character.

This does not happen with coltypes<-, as it's not part of base package and doesn't have an (ANY,ANY) signature.

coltypes(irisDF) <- 1
Error in (function (classes, fdef, mtable) :
unable to find an inherited method for function ‘coltypes<-’ for signature ‘"DataFrame", "numeric"’

Therefore, I believe we do need to do this data type check inside colnames<-.

[sun-rui]

It is interesting. @felixcheung, do you know the reason behind this?

[felixcheung]

I think in this case the base:: definition is implemented in such a way that it should (or at least it attempts to) handle different parameter types:

> getMethod("colnames<-")
Method Definition (Class "derivedDefaultMethod"):

function (x, value)
{
    if (is.data.frame(x)) {
        names(x) <- value
    }
    else {
        dn <- dimnames(x)
        if (is.null(dn)) {
            if (is.null(value))
                return(x)
            if ((nd <- length(dim(x))) < 2L)
                stop("attempt to set 'colnames' on an object with less than two dimensions")
            dn <- vector("list", nd)
        }
        if (length(dn) < 2L)
            stop("attempt to set 'colnames' on an object with less than two dimensions")
        if (is.null(value))
            dn[2L] <- list(NULL)
        else dn[[2L]] <- value
        dimnames(x) <- dn
    }
    x
}
<bytecode: 0x34c9058>
<environment: namespace:base>

Signatures:
        x     value
target  "ANY" "ANY"
defined "ANY" "ANY"

So from what I can see the error you see actually comes from the base implement, as "sort of" expected. Though I'm ok if we are to add explicit checks to make the error more clear for the user.

[olarayej]

Thanks @felixcheung for investigating this further!

[olarayej]

Jenkins, retest please

[SparkQA]

Test build #51676 has finished for PR 11220 at commit 07e541b.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[sun-rui]

LGTM

[shivaram]

LGTM. Merging this.

[olarayej]

Thanks, @shivaram!