[SPARK-24259][SQL] ArrayWriter for Arrow produces wrong output #21312

viirya · 2018-05-13T00:36:51Z

What changes were proposed in this pull request?

Right now ArrayWriter used to output Arrow data for array type, doesn't do clear or reset after each batch. It produces wrong output.

How was this patch tested?

Added test.

viirya · 2018-05-13T00:55:19Z

cc @HyukjinKwon @BryanCutler

HyukjinKwon · 2018-05-13T02:14:15Z

sql/core/src/main/scala/org/apache/spark/sql/execution/arrow/ArrowWriter.scala

  override def reset(): Unit = {
    super.reset()
    elementWriter.reset()
+    valueVector.clear()


Looks @BryanCutler added reset() interface in 0.9.0 mentioned in:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/arrow/ArrowWriter.scala

Line 132 in eb386be

// TODO: reset() should be in a common interface

at apache/arrow@4dbce60 and https://issues.apache.org/jira/browse/ARROW-1962

but if we think about backporting, probably I guess we can go this way as a bug fix as is? Roughly looks making sense.

Would it be also safe to do:

valueVector match { case fixedWidthVector: BaseFixedWidthVector => fixedWidthVector.reset() case variableWidthVector: BaseVariableWidthVector => variableWidthVector.reset() case repeatedValueVector: BaseRepeatedValueVector => repeatedValueVector.clear() case _ => }

? @icexelloss, @BryanCutler and @viirya?

Yeah, I think so.

I've also noticed that @BryanCutler added reset to ListVector. But we can only use clear for now.

HyukjinKwon

LGTM from my side but let me leave this to @BryanCutler and @icexelloss.

viirya · 2018-05-13T03:30:06Z

Thanks @HyukjinKwon

SparkQA · 2018-05-13T04:19:23Z

Test build #90544 has finished for PR 21312 at commit 093728e.

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

SparkQA · 2018-05-13T06:56:38Z

Test build #90547 has finished for PR 21312 at commit 48ef560.

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

BryanCutler · 2018-05-14T06:28:48Z

Thanks for catching this @viirya! Looks good from a first glance, but my only concern is that clear() will release the vector buffers, where reset() just zeros them out. Let me look into that a little further tomorrow and make sure it won't cause a problem.

icexelloss · 2018-05-14T13:24:56Z

@viirya Thanks for catching this!

I think we have many tests that excise the array types. I am curious why this is not caught by existing tests, e.g:
https://github.com/apache/spark/blob/master/python/pyspark/sql/tests.py#L4645

viirya · 2018-05-14T13:31:02Z

@icexelloss It only happens when there are more than one batch in each partition. Existing tests do not hit this condition. That is why the added test here is doing a repartition:

df = self.data.withColumn("arr", array(col("id"))).repartition(1, "id")

BryanCutler · 2018-05-14T18:05:37Z

@viirya I looked into it a bit more and calling clear() won't cause any problems but it does trigger a reallocation of the vector buffers the next time writing. What do you think about changing this to do a manual "reset" so that the buffers can be reused? It just needs to zero out the buffers and set the value count to 0, so something like this:

val buffers = repeatedValueVector.getBuffers(false)
buffers.foreach(buf => buf.setZero(0, buf.capacity()))
repeatedValueVector.setValueCount(0)

Once we upgrade to Arrow 0.10.0, this can be cleaned up because there is a common interface to reset(). I think we should definitely get this backported to the 2.3 branch too.

icexelloss · 2018-05-14T18:10:13Z

Agree on both points.

viirya · 2018-05-14T23:08:05Z

@BryanCutler I have such thought but wondered if it is good to do that. If you @HyukjinKwon @icexelloss are also agreed on manual reset like this, I'm fine with it.

HyukjinKwon · 2018-05-15T00:07:55Z

I'm okay with either way.

viirya · 2018-05-15T00:11:04Z

Ok. I will use manual reset for now and leave a TODO comment.

BryanCutler · 2018-05-15T00:29:20Z

It looks like the ListVector also needs setLastSet to be called with 0, which is only in ListVector. This is fine though, since ListVector is the only vector extending BaseRepeatedValueVector

case listVector: ListVector =>
        val buffers = listVector.getBuffers(false)
        buffers.foreach(buf => buf.setByte(0, buf.capacity()))
        listVector.setValueCount(0)
        listVector.setLastSet(0)

viirya · 2018-05-15T04:00:50Z

Not sure why, but previously calling ListVector.clear, I must change the reset order to reset element writer first to pass the test:

 override def reset(): Unit = {
+    elementWriter.reset()
     super.reset()
-    elementWriter.reset()
   }

Now with manual reset, this order doesn't affect test result anymore. I respect original order and restore it back.

SparkQA · 2018-05-15T06:34:33Z

Test build #90618 has finished for PR 21312 at commit a2d1444.

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

SparkQA · 2018-05-15T07:05:01Z

Test build #90621 has finished for PR 21312 at commit 400f605.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-05-15T07:05:56Z

retest this please.

cloud-fan · 2018-05-15T07:12:38Z

sql/core/src/main/scala/org/apache/spark/sql/execution/arrow/ArrowWriter.scala

    valueVector match {
      case fixedWidthVector: BaseFixedWidthVector => fixedWidthVector.reset()
      case variableWidthVector: BaseVariableWidthVector => variableWidthVector.reset()
+      case listVector: ListVector =>


good catch! So this bug was there since the day 1 when we have arrow writer, right?

Yeah, I think so.

SparkQA · 2018-05-15T10:51:32Z

Test build #90626 has finished for PR 21312 at commit 400f605.

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

icexelloss

LGTM!

cloud-fan · 2018-05-15T14:13:45Z

thanks, merging to master/2.3!

viirya · 2018-05-15T14:14:08Z

Thanks @HyukjinKwon @BryanCutler @cloud-fan @icexelloss

## What changes were proposed in this pull request? Right now `ArrayWriter` used to output Arrow data for array type, doesn't do `clear` or `reset` after each batch. It produces wrong output. ## How was this patch tested? Added test. Author: Liang-Chi Hsieh <[email protected]> Closes #21312 from viirya/SPARK-24259. (cherry picked from commit d610d2a) Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? Right now `ArrayWriter` used to output Arrow data for array type, doesn't do `clear` or `reset` after each batch. It produces wrong output. ## How was this patch tested? Added test. Author: Liang-Chi Hsieh <[email protected]> Closes apache#21312 from viirya/SPARK-24259.

BryanCutler · 2018-05-16T23:24:57Z

Not sure why, but previously calling ListVector.clear, I must change the reset order to reset element writer first to pass the test

@viirya I looked into this and found it to be a bug in Arrow when clearing, then reusing a vector. I filed https://issues.apache.org/jira/browse/ARROW-2594. Changing the order of the elements only masked the problem, because otherwise it would reuse a buffer with incorrect values.

This won't happen with the change here using the manual reset, so we should be good.

viirya · 2018-05-16T23:27:51Z

@BryanCutler Thanks! I'm happy we can identify this possible bug. Looking forward to the fixing.

Call clear after each batch.

093728e

HyukjinKwon reviewed May 13, 2018

View reviewed changes

Address comment.

48ef560

HyukjinKwon approved these changes May 13, 2018

View reviewed changes

viirya mentioned this pull request May 14, 2018

[SPARK-24242][SQL] RangeExec should have correct outputOrdering and outputPartitioning #21291

Closed

Use manual reset and add a TODO comment.

400f605

viirya force-pushed the SPARK-24259 branch from a2d1444 to 400f605 Compare May 15, 2018 03:45

cloud-fan reviewed May 15, 2018

View reviewed changes

icexelloss approved these changes May 15, 2018

View reviewed changes

asfgit closed this in d610d2a May 15, 2018

BryanCutler mentioned this pull request May 16, 2018

ARROW-2594: [Java] When realloc Vectors, zero out all unfilled bytes of new buffer apache/arrow#2054

Closed

viirya deleted the SPARK-24259 branch December 27, 2023 18:35

[SPARK-24259][SQL] ArrayWriter for Arrow produces wrong output #21312

[SPARK-24259][SQL] ArrayWriter for Arrow produces wrong output #21312

Uh oh!

Conversation

viirya commented May 13, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya commented May 13, 2018

Uh oh!

HyukjinKwon May 13, 2018

Choose a reason for hiding this comment

Uh oh!

viirya May 13, 2018

Choose a reason for hiding this comment

Uh oh!

viirya May 13, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented May 13, 2018

Uh oh!

SparkQA commented May 13, 2018

Uh oh!

SparkQA commented May 13, 2018

Uh oh!

BryanCutler commented May 14, 2018

Uh oh!

icexelloss commented May 14, 2018

Uh oh!

viirya commented May 14, 2018

Uh oh!

BryanCutler commented May 14, 2018

Uh oh!

icexelloss commented May 14, 2018

Uh oh!

viirya commented May 14, 2018

Uh oh!

HyukjinKwon commented May 15, 2018

Uh oh!

viirya commented May 15, 2018

Uh oh!

BryanCutler commented May 15, 2018

Uh oh!

viirya commented May 15, 2018

Uh oh!

SparkQA commented May 15, 2018

Uh oh!

SparkQA commented May 15, 2018

Uh oh!

viirya commented May 15, 2018

Uh oh!

cloud-fan May 15, 2018

Choose a reason for hiding this comment

Uh oh!

viirya May 15, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 15, 2018

Uh oh!

icexelloss left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented May 15, 2018

Uh oh!

viirya commented May 15, 2018

Uh oh!

BryanCutler commented May 16, 2018

Uh oh!

viirya commented May 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!