[SPARK-12227][SQL] Support drop multiple columns specified by Column class in DataFrame API

[HyukjinKwon]

https://issues.apache.org/jira/browse/SPARK-12227

In this PR, I added the support to drop multiple columns by reference. This was only supported with the string names.

As varargs does not recognise the type for multuple variables, I changed drop(colNames: String*) to drop(colName: String, colNames: String*) correspondingly to orderBy(sortCol: String, sortCols: String*) and sort(sortCol: String, sortCols: String*).

Accordingly, the call in drop(colName: String) was changed.

[SparkQA]

Test build #47412 has finished for PR 10218 at commit 2223526.

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

[aray]

Doesn't this break binary compatibility?

[HyukjinKwon]

Would you please tell me a bit more about what you meant? I am not used to binary compatibility issues and to me it looks fine about the compatibility between compiled ones with different Spark versions.

One thing I am a bit worried is the ambiguity of parameters (in terms of Java interoperability), which I could fix by adding an additional parameter (if this is a problem).

[HyukjinKwon]

Just FYI, I am not too sure about all the versions but at least this works fine at Java 7 and Scala 2.10.4.

• Scala

class TestCompatibility {
  def test(a: String): Unit = {
    test(a, Seq(): _ *)
  }
  @varargs
  def test(a: String, b: String*): Unit = {
    (a +: b).foreach(println)
  }
  def test(a: Int): Unit = {
    test(Seq(a) : _ *)
  }
  @varargs
  def test(a: Int*): Unit = {
    a.foreach(println)
  }
}

• Java

public class Test {
    public static void main(String[] args) {
        new TestCompatibility().test("a");
        new TestCompatibility().test("a", "b");
        new TestCompatibility().test("a", "b", "c");
        new TestCompatibility().test(1);
        new TestCompatibility().test(1, 2);
        new TestCompatibility().test(1, 2, 3);
    }
}

[aray]

Never mind, the annotation is wrong it was added in #9862 which did not get merged into 1.6 so no worry about changing the method signature.

[marmbrus]

I'm not sure this is worth the complexity. I think most users will only ever drop by name (since dropping a complex expression doesn't really make sense), and in that case constructing a column is strictly more typing.

[HyukjinKwon]

I see. I added this as sort, cube, select and etc do the same thing for supporting it by name and expression. Although I am not greatly insightful for this DataFrame APIs, I feel like some users would feel it is inconsistent if most APIs support this by name and expression but this does not.

Also, I think this might not be so complicated. I just added a loop and made drop(col: Column) call this but I accept that this might look complicated (in terms of readability).

So, it looks like consistency vs readability for me.

[marmbrus]

More functions for the sake of more functions does not make the API easier to use. We should not add functions just to be consistent, we should only add them if they are going to be used.

The complexity come from the fact that users who have a seq of string have to do something like this now:

val toDrop = Seq("col1", "col2", "col3")
df.drop(toDrop.head, toDrop.tail)

Can you construct a case when you would have columns instead of strings that you wanted to drop? In the test case is strictly more typing to use this API compared to the one that exists already. This doesn't seem worth complicating the case above.

val df = src.drop("a", "b")
val df = src.drop(src("a"), src("b"))

[HyukjinKwon]

Thanks for your detailed explanation! Then does this mean closing this?

[marmbrus]

yes please.