Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Dec 22, 2015

This also adds a test to validate that serialization works for all
Binary objects that are already test cases.

@lw-lin
Copy link
Contributor

lw-lin commented Jan 27, 2016

+1, LGTM. Good to have testSerializable() for unit tests!

So, @rdblue , should we have more committers review on this, or can we get this committed? Cheers.

@rdblue
Copy link
Contributor Author

rdblue commented Jan 27, 2016

Thanks for reviewing, @proflin! We will need a committer to have a look as well, but it really helps to have more than one person review. Like we said in the sync-up today, we love it when non-committers help out with reviews because it makes everything go faster.

@lw-lin
Copy link
Contributor

lw-lin commented Jan 27, 2016

Sure. I'm familiar with the parquet-format/parquet-mr code base, and I would love to review more PRs. Things are going fast these days :)

BTW congratulations on your new journey at Netflix :-)

@julienledem
Copy link
Member

+1
Thanks for the review @proflin

This also adds a test to validate that serialization works for all
Binary objects that are already test cases.
@rdblue rdblue force-pushed the PARQUET-415-fix-bytebuffer-binary-serialization branch from 65bdcf0 to 4e75d54 Compare February 3, 2016 19:53
@asfgit asfgit closed this in 0a711eb Feb 3, 2016
piyushnarang pushed a commit to piyushnarang/parquet-mr that referenced this pull request Jun 15, 2016
This also adds a test to validate that serialization works for all
Binary objects that are already test cases.

Author: Ryan Blue <blue@apache.org>

Closes apache#305 from rdblue/PARQUET-415-fix-bytebuffer-binary-serialization and squashes the following commits:

4e75d54 [Ryan Blue] PARQUET-415: Fix ByteBuffer Binary serialization.
rdblue added a commit to rdblue/parquet-mr that referenced this pull request Jan 6, 2017
This also adds a test to validate that serialization works for all
Binary objects that are already test cases.

Author: Ryan Blue <blue@apache.org>

Closes apache#305 from rdblue/PARQUET-415-fix-bytebuffer-binary-serialization and squashes the following commits:

4e75d54 [Ryan Blue] PARQUET-415: Fix ByteBuffer Binary serialization.
@jinxing64
Copy link

@rdblue
Thanks for fixing this problem, may I ask a question?

  1. What parquet version includes this change? is it 1.9.0?
  2. Without this change, what exception will be thrown? is it below?
Caused by: java.nio.BufferUnderflowException
        at java.nio.HeapByteBuffer.get(HeapByteBuffer.java:151)
        at java.nio.ByteBuffer.get(ByteBuffer.java:715)
        at org.apache.parquet.io.api.Binary$ByteBufferBackedBinary.getBytes(Binary.java:405)
        at org.apache.parquet.io.api.Binary$ByteBufferBackedBinary.getBytesUnsafe(Binary.java:414)
        at org.apache.parquet.io.api.Binary$ByteBufferBackedBinary.writeObject(Binary.java:484)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at java.io.ObjectStreamClass.invokeWriteObject(ObjectStreamClass.java:1028)
        at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1496)
        at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
        at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
        at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
        at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
......

@rdblue
Copy link
Contributor Author

rdblue commented Apr 10, 2018

@jinxing64, this is fixed in 1.9.0. It could result in the trace you pasted, I think. I'm not sure if that's actually happening though.

@jinxing64
Copy link

@rdblue
Thanks a lot !

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