[SPARK-3121] Wrong implementation of implicit bytesWritableConverter #2712

james64 · 2014-10-08T14:00:04Z

val path = ... //path to seq file with BytesWritable as type of both key and value
val file = sc.sequenceFileArray[Byte],Array[Byte]
file.take(1)(0)._1

This prints incorrect content of byte array. Actual content starts with correct one and some "random" bytes and zeros are appended. BytesWritable has two methods:

getBytes() - return content of all internal array which is often longer then actual value stored. It usually contains the rest of previous longer values

copyBytes() - return just begining of internal array determined by internal length property

It looks like in implicit conversion between BytesWritable and Array[byte] getBytes is used instead of correct copyBytes.

@dbtsai

AmplabJenkins · 2014-10-08T14:02:10Z

Can one of the admins verify this patch?

dbtsai · 2014-10-08T14:02:19Z

Jenkins, please start the test.

SparkQA · 2014-10-08T16:35:56Z

QA tests have started for PR 2712 at commit 480f9cd.

This patch merges cleanly.

sryza · 2014-10-08T16:40:06Z

Great catch.

A concern is that calling Array#take requires an implicit conversion, which has some performance impact that might be unacceptable for this method that can get called in a tight loop.

http://villane.wordpress.com/2008/02/02/learning-scala-performance-impact-of-implicit-conversions/

SparkQA · 2014-10-08T17:37:46Z

QA tests have finished for PR 2712 at commit 480f9cd.

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

james64 · 2014-10-08T22:55:25Z

Originaly I wanted to just replace getBytes method with copyBytes. It is available in newer versions of api but I found an older version is imported in spark. I am not very familiar with what hadoop api is used in spark yet. So do you suggest to implement it without usage of take method?

sryza · 2014-10-09T07:40:52Z

Hmm, yeah, copyBytes is no good if it doesn't appear in Hadoop 1.

My suggestion would be to use from copyOfRange from java.util.Arrays.

james64 · 2014-10-09T15:24:12Z

I pushed the new version. I guess jenkins test will kick out automatically right?

sryza · 2014-10-09T20:29:28Z

Jenkins, test this please.

AmplabJenkins · 2014-10-09T20:32:15Z

Can one of the admins verify this patch?

dbtsai · 2014-10-10T08:52:26Z

Jenkins, test this please.

JoshRosen · 2014-10-10T21:17:23Z

Jenkins, this is ok to test.

SparkQA · 2014-10-10T21:24:36Z

QA tests have started for PR 2712 at commit f92ffa6.

This patch merges cleanly.

SparkQA · 2014-10-10T21:25:39Z

QA tests have finished for PR 2712 at commit f92ffa6.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-10-10T21:25:40Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21602/Test FAILed.

sryza · 2014-10-10T21:27:33Z

core/src/main/scala/org/apache/spark/SparkContext.scala

@@ -1409,7 +1411,7 @@ object SparkContext extends Logging {
    simpleWritableConverter[Boolean, BooleanWritable](_.get)

  implicit def bytesWritableConverter(): WritableConverter[Array[Byte]] = {
-    simpleWritableConverter[Array[Byte], BytesWritable](_.getBytes)
+    simpleWritableConverter[Array[Byte], BytesWritable](bw => Arrays.copyOfRange(bw.getBytes, 0, bw.getLength))


it looks like this goes past 100 characters

SparkQA · 2014-10-10T22:09:39Z

QA tests have started for PR 2712 at commit 406e26c.

This patch merges cleanly.

SparkQA · 2014-10-10T23:03:56Z

QA tests have finished for PR 2712 at commit 406e26c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-10-10T23:03:59Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21603/Test FAILed.

dbtsai · 2014-10-11T00:40:12Z

It's failing at FlumeStreamSuite.scala:109 which seems to be unrelated to this patch.

sryza · 2014-10-11T00:43:48Z

One more nit: the added java import should go with the other java imports.

sryza · 2014-10-11T00:43:56Z

Otherwise, LGTM

james64 · 2014-10-11T09:02:54Z

Can it be that test Flume test failed due to upstream changes? It is passing for me locally now.

SparkQA · 2014-10-11T09:04:39Z

QA tests have started for PR 2712 at commit 1b20d51.

This patch merges cleanly.

SparkQA · 2014-10-11T10:07:53Z

QA tests have finished for PR 2712 at commit 1b20d51.

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

AmplabJenkins · 2014-10-11T10:07:57Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21641/Test PASSed.

JoshRosen · 2014-10-11T19:05:54Z

That particular Flume test is known to be flaky; I think that TD is working on a rewrite / fix for that test suite.

JoshRosen · 2014-10-11T19:08:11Z

core/src/test/scala/org/apache/spark/SparkContextSuite.scala

+import org.apache.hadoop.io.BytesWritable
+
+class SparkContextSuite extends FunSuite {
+  test("test of writing spark scala test") {


This test could use a better name. I'd also add a comment, like // Regression test for SPARK-3121 to help readers link this back to the JIRA.

JoshRosen · 2014-10-11T19:33:35Z

Actually, ignore my earlier (deleted) comments; this looks like a valid issue (see HADOOP-6298: "BytesWritable#getBytes is a bad name that leads to programming mistakes").

JoshRosen · 2014-10-11T19:37:46Z

core/src/main/scala/org/apache/spark/SparkContext.scala

@@ -1409,7 +1410,9 @@ object SparkContext extends Logging {
    simpleWritableConverter[Boolean, BooleanWritable](_.get)

  implicit def bytesWritableConverter(): WritableConverter[Array[Byte]] = {
-    simpleWritableConverter[Array[Byte], BytesWritable](_.getBytes)
+    simpleWritableConverter[Array[Byte], BytesWritable](bw =>
+      Arrays.copyOfRange(bw.getBytes, 0, bw.getLength)


Could you add a one-line comment here that explains why we need to make this copy?

JoshRosen · 2014-10-11T19:39:48Z

This looks good to me; sorry for my earlier confusion. If you add a comment and change the name of the test, I'll merge this and cherry-pick it back into branch-1.1 and branch-1.0.

james64 · 2014-10-11T23:19:43Z

Sorry for the test name. Now it should be all fine including commets.

SparkQA · 2014-10-11T23:24:43Z

QA tests have started for PR 2712 at commit f85d24c.

This patch merges cleanly.

SparkQA · 2014-10-12T00:27:56Z

QA tests have finished for PR 2712 at commit f85d24c.

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

AmplabJenkins · 2014-10-12T00:27:59Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21649/Test PASSed.

JoshRosen · 2014-10-13T04:54:14Z

This looks great; thanks for adding the comments. I'm going to merge this into master and backport it to branch-1.0 and branch-1.1.

val path = ... //path to seq file with BytesWritable as type of both key and value val file = sc.sequenceFile[Array[Byte],Array[Byte]](path) file.take(1)(0)._1 This prints incorrect content of byte array. Actual content starts with correct one and some "random" bytes and zeros are appended. BytesWritable has two methods: getBytes() - return content of all internal array which is often longer then actual value stored. It usually contains the rest of previous longer values copyBytes() - return just begining of internal array determined by internal length property It looks like in implicit conversion between BytesWritable and Array[byte] getBytes is used instead of correct copyBytes. dbtsai Author: Jakub Dubovský <james64@inMail.sk> Author: Dubovsky Jakub <dubovsky@avast.com> Closes #2712 from james64/3121-bugfix and squashes the following commits: f85d24c [Jakub Dubovský] Test name changed, comments added 1b20d51 [Jakub Dubovský] Import placed correctly 406e26c [Jakub Dubovský] Scala style fixed f92ffa6 [Dubovsky Jakub] performance tuning 480f9cd [Dubovsky Jakub] Bug 3121 fixed (cherry picked from commit fc616d5) Signed-off-by: Josh Rosen <joshrosen@apache.org>

Bug 3121 fixed

480f9cd

performance tuning

f92ffa6

sryza reviewed Oct 10, 2014
View reviewed changes

Scala style fixed

406e26c

Import placed correctly

1b20d51

JoshRosen reviewed Oct 11, 2014
View reviewed changes

Test name changed, comments added

f85d24c

asfgit closed this in fc616d5 Oct 13, 2014

james64 deleted the 3121-bugfix branch October 13, 2014 11:30

This was referenced Dec 26, 2014

[SPARK-4969][STREAMING][PYTHON] Add binaryRecords to streaming #3803

Closed

[SPARK-2759][CORE] Generic Binary File Support in Spark #1658

Closed

[SPARK-3121] Wrong implementation of implicit bytesWritableConverter #2712

[SPARK-3121] Wrong implementation of implicit bytesWritableConverter #2712

Conversation

james64 commented Oct 8, 2014

AmplabJenkins commented Oct 8, 2014

dbtsai commented Oct 8, 2014

SparkQA commented Oct 8, 2014

sryza commented Oct 8, 2014

SparkQA commented Oct 8, 2014

james64 commented Oct 8, 2014

sryza commented Oct 9, 2014

james64 commented Oct 9, 2014

sryza commented Oct 9, 2014

AmplabJenkins commented Oct 9, 2014

dbtsai commented Oct 10, 2014

JoshRosen commented Oct 10, 2014

SparkQA commented Oct 10, 2014

SparkQA commented Oct 10, 2014

AmplabJenkins commented Oct 10, 2014

sryza Oct 10, 2014

Choose a reason for hiding this comment

SparkQA commented Oct 10, 2014

SparkQA commented Oct 10, 2014

AmplabJenkins commented Oct 10, 2014

dbtsai commented Oct 11, 2014

sryza commented Oct 11, 2014

sryza commented Oct 11, 2014

james64 commented Oct 11, 2014

SparkQA commented Oct 11, 2014

SparkQA commented Oct 11, 2014

AmplabJenkins commented Oct 11, 2014

JoshRosen commented Oct 11, 2014

JoshRosen Oct 11, 2014

Choose a reason for hiding this comment

JoshRosen commented Oct 11, 2014

JoshRosen Oct 11, 2014

Choose a reason for hiding this comment

JoshRosen commented Oct 11, 2014

james64 commented Oct 11, 2014

SparkQA commented Oct 11, 2014

SparkQA commented Oct 12, 2014

AmplabJenkins commented Oct 12, 2014

JoshRosen commented Oct 13, 2014