[SPARK-12604] [CORE] Java count(AprroxDistinct)ByKey methods return Scala Long not Java

[srowen]

Change Java countByKey, countApproxDistinctByKey return types to use Java Long, not Scala; update similar methods for consistency on java.long.Long.valueOf with no API change

[SparkQA]

Test build #2297 has finished for PR 10554 at commit 1a27421.

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

[rxin]

Does this actually change anything w.r.t. bytecode singature?

[srowen]

I don't think it does. It may cause source incompatibility since the generic type changes. You can see an example of a fix/change that can happen in the caller in the example that changed.

[srowen]

Changed for consistency; valueOf is slightly more efficient than the constructor since the JDK actually caches Long objects for small values.

[rxin]

+1

[rxin]

@srowen If this doesn't change any signature, I think it actually makes things slower by adding another layer of iterators. Maybe it'd make more sense to just change the signature rather than doing actual "conversions".

[srowen]

It doesn't compile that way though since the values are Scala Longs and the signature says Java Longs. One way or the other, such a conversion has to happen somewhere. If it works for anyone it's because they're already doing this in the calling code.

[rxin]

One thing I don't understand is how can these methods return scala types at the bytecode level? Scala Long is just a primitive long. All the places you find are generics, which cannot be primitive types, and as a result I assume they are all java boxed types?

[srowen]

Yeah that's a good point -- let me see if I can understand that better. At some level, it has to be an Object and not a long in order to be part of a java.util.Map, so something is transforming something. It may be that implicits convert it to a java.lang.Long in the end, maybe via RichLong, which would be pretty good news -- this is just then fixing the signature and making explicit some implicit conversion and maybe even making it more efficient. Let me experiment a bit

[srowen]

@rxin so I tried unpacking this code in IntelliJ:

val m = new java.util.HashMap[String, java.lang.Long]()
val l = 1L
m.put("foo", l)

... and it tells me that the implicit conversion that is applied here is Predef.long2Long:

implicit def long2Long(x: Long)           = java.lang.Long.valueOf(x)

It looks like this is the same underlying transformation that happens in SerializableMapWrapper and mapAsSerializableJavaMap already. So it looks to me like this is already the implicit transformation being applied anyway.

Right now the returned type of these methods in Java is Map<K, Object> and the wrapper does the conversion on the fly in get(). I believe. I think to make it a real Map<K, Long> requires making the transformation upfront -- well, short of making some other new special purpose wrapper.

WDYT?

[srowen]

... or I suppose the implementation can just cast to java.util.Map[K, java.lang.Long] since that's safe. It's less change, and mimics what callers are doing anyway in Java.

[rxin]

Yea your latest suggestion sounds great (a very tiny change).

[srowen]

@rxin OK I almost did that. I realized that JavaRDD.countByValue already does a mapValues. I left countByKey to act the same way, doing the mapping. Other methods that return JavaPairRDD went back to doing a cast. (Although similar methods in the streaming API actually also do a mapValues.) I also tried to use JLong vs jl.Long consistently within a file without going overboard; both of these files use about every different form. I think the result is more consistent internally, but WDYT? I'm neutral, and would further change things while keeping them consistent if anyone had a preference.

[SparkQA]

Test build #48759 has finished for PR 10554 at commit 39fa6e7.

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

[rxin]

fwiw, i actually think spelling out java.util.Map is more clear ...

[SparkQA]

Test build #48847 has finished for PR 10554 at commit 293b5e4.

• This patch fails from timeout after a configured wait of 250m.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #2335 has finished for PR 10554 at commit 293b5e4.

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

[rxin]

I'm going to merge this. Thanks Sean.

[rxin]

@srowen how come there is still a mapValues here?

[srowen]

See my reply at #10554 (comment) -- it was because I then saw this was how countByKey had been implemented. I picked consistency, but it's as reasonable to change countByKey I suppose. Do you feel strongly about it one way or the other?