Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-22067][SQL] ArrowWriter should use position when setting UTF8String ByteBuffer #19284

Conversation

BryanCutler
Copy link
Member

@BryanCutler BryanCutler commented Sep 19, 2017

What changes were proposed in this pull request?

The ArrowWriter StringWriter was setting Arrow data using a position of 0 instead of the actual position in the ByteBuffer. This was currently working because of a bug ARROW-1443, and has been fixed as of
Arrow 0.7.0. Testing with this version revealed the error in ArrowConvertersSuite test string conversion.

How was this patch tested?

Existing tests, manually verified working with Arrow 0.7.0

@BryanCutler
Copy link
Member Author

BryanCutler commented Sep 19, 2017

@ueshin and @icexelloss this came up while testing with Arrow 0.7.0. It seems that when Spark gets row data as a UTF8String ByteBuffer, the data can start at an offset which becomes the ByteBuffer position when this line is called https://github.com/apache/spark/blob/master/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java#L171

@icexelloss
Copy link
Contributor

LGTM. What's the Arrow bug you mentioned?

@BryanCutler
Copy link
Member Author

BryanCutler commented Sep 19, 2017

Ooops, I referenced the wrong JIRA, it was ARROW-1443 PR: apache/arrow#1022 ArrowBuf.setBytes was not using the destination buffer properly

@SparkQA
Copy link

SparkQA commented Sep 20, 2017

Test build #81952 has finished for PR 19284 at commit 5ac572d.

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

@ueshin
Copy link
Member

ueshin commented Sep 20, 2017

LGTM.

@ueshin
Copy link
Member

ueshin commented Sep 20, 2017

Thanks! merging to master.

@asfgit asfgit closed this in 718bbc9 Sep 20, 2017
@BryanCutler
Copy link
Member Author

Thanks @ueshin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants