[SPARK-4459] Change groupBy type parameter from K to U #3327
Conversation
alokito
changed the title
alokito
changed the title
alokito
changed the title
|
Can one of the admins verify this patch? |
|
LGTM. The question is whether this changes the binary or source compatibility. I don't think it changes source compatibility since it merely loosens a restriction. I don't think it would change the binary signature either? but MIMA will help know for sure. Also, I think |
| @@ -322,6 +322,27 @@ public Boolean call(Integer x) { | |||
| Assert.assertEquals(2, Iterables.size(oddsAndEvens.lookup(true).get(0))); // Evens | |||
| Assert.assertEquals(5, Iterables.size(oddsAndEvens.lookup(false).get(0))); // Odds | |||
| } | |||
|
|
|||
| @Test | |||
|
|
|
I may have time to add some tests and patch keyBy and partitionBy tomorrow. |
|
I addressed keyBy, but I think partitionBy does not actually have this issue, as the Partitioner abstract class does not take a type parameter - implementations such as RangePartitioner use asInstanceOf in getPartition. |
|
It looks like you might have run into a Scala compiler bug, since in principle it shouldn't matter what we name that type parameter, since it's in a different scope than other type parameters. This looks a lot like https://issues.scala-lang.org/browse/SI-6057, which I hit while implementing the Java API and @tdas ran into while working on the Java API for Spark Streaming. |
|
If you've got time, do you mind fixing up the indentation / formatting issues in the test suite so that I can merge this? If not, just let me know and I'll do it myself. |
| Function<scala.Tuple2<Integer, Integer>, String> areOdd = new Function<scala.Tuple2<Integer, Integer>, String>() { | ||
| @Override | ||
| public String call(scala.Tuple2<Integer, Integer> x) { | ||
| return ""+(x._1 +x._2); |
There was a problem hiding this comment.
The spacing here is messy. Also, "" + x is messy; just do x.toString() instead if you want to convert an object in a string.
|
I downloaded this and tested it out; everything looks fine, modulo these formatting issues. I've fixed the style issues myself and am going to merge this into (I ran the MiMa tests locally and this passes) |
Please see https://issues.apache.org/jira/browse/SPARK-4459 Author: Saldanha <[email protected]> Closes #3327 from alokito/master and squashes the following commits: 54b1095 [Saldanha] [SPARK-4459] changed type parameter for keyBy from K to U d5f73c3 [Saldanha] [SPARK-4459] added keyBy test 316ad77 [Saldanha] SPARK-4459 changed type parameter for groupBy from K to U. 62ddd4b [Saldanha] SPARK-4459 added failing unit test (cherry picked from commit 743a889) Signed-off-by: Josh Rosen <[email protected]>
Please see https://issues.apache.org/jira/browse/SPARK-4459 Author: Saldanha <[email protected]> Closes #3327 from alokito/master and squashes the following commits: 54b1095 [Saldanha] [SPARK-4459] changed type parameter for keyBy from K to U d5f73c3 [Saldanha] [SPARK-4459] added keyBy test 316ad77 [Saldanha] SPARK-4459 changed type parameter for groupBy from K to U. 62ddd4b [Saldanha] SPARK-4459 added failing unit test (cherry picked from commit 743a889) Signed-off-by: Josh Rosen <[email protected]>
|
Thanks for fixing the style issues, I meant to but it's been a tough week. On Dec 4, 2014, at 17:57, Josh Rosen [email protected] wrote: I downloaded this and tested it out; everything looks fine, modulo these (I ran the MiMa tests locally and this passes) — |
Please see https://issues.apache.org/jira/browse/SPARK-4459