| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@Yicong-Huang can you please rebase the PR ? Thanks ! |
Sorry, something went wrong.
I think you are referring the writers in Spark. It is out of context here and not related to the root cause. We should update the description to explain the issue clearly. The offset buffers are actually allocated properly. But during IPC serialization, they are ignored. public long readableBytes() {
return writerIndex - readerIndex;
}So when ListVector.setReaderAndWriterIndex() sets writerIndex(0) and readerIndex(0), readableBytes() returns 0 - 0 = 0. Then when MessageSerializer.writeBatchBuffers() calls WriteChannel.write(buffer), it writes 0 bytes. So the flow is:
|
Sorry, something went wrong.
Sorry, something went wrong.
| validityBuffer.writerIndex(BitVectorHelper.getValidityBufferSizeFromCount(valueCount)); | ||
| offsetBuffer.writerIndex((valueCount + 1) * OFFSET_WIDTH); | ||
| } | ||
| validityBuffer.writerIndex(BitVectorHelper.getValidityBufferSizeFromCount(valueCount)); |
There was a problem hiding this comment.
When valueCount == 0, I think validity buffer writer index should be validityBuffer.writerIndex(0);.
Sorry, something went wrong.
There was a problem hiding this comment.
I believe when valueCount==0, BitVectorHelper.getValidityBufferSizeFromCount(valueCount) also returns 0, so it is equivalent. The current version might be simpler. If you prefer an if branch to handle it separately, I can also apply it.
Sorry, something went wrong.
There was a problem hiding this comment.
I added the if branch back to handle valueCount==0 case. But I still think it is not necessary?
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, if BitVectorHelper.getValidityBufferSizeFromCount(valueCount) returns 0 for valueCount == 0, then it is okay.
Sorry, something went wrong.
| // Allocate outer only - simulates case where inner is never written to | ||
| outerList.allocateNew(); | ||
| outerList.setValueCount(0); |
There was a problem hiding this comment.
I think innerList should also call allocateNew? Not allocated is different to not written.
Sorry, something went wrong.
There was a problem hiding this comment.
added
Sorry, something went wrong.
| // Only allocate level0 - simulates case where all nested levels are empty | ||
| level0.allocateNew(); | ||
| level0.setValueCount(0); |
There was a problem hiding this comment.
ditto
Sorry, something went wrong.
There was a problem hiding this comment.
added!
Sorry, something went wrong.
| try (LargeListVector outerList = LargeListVector.empty("outer", allocator)) { | ||
| // Setup LargeList<LargeList<Int>> | ||
| outerList.addOrGetVector(FieldType.nullable(MinorType.LARGELIST.getType())); | ||
| LargeListVector innerList = (LargeListVector) outerList.getDataVector(); | ||
| innerList.addOrGetVector(FieldType.nullable(MinorType.INT.getType())); |
There was a problem hiding this comment.
I looked these tests again. I think this nested structure is not necessary for the unit tests here. It only matter for our usage at Spark side (on the ArrowWriters). But for here, we just need to make sure that a ListVector/LargeListVector has meaningful and correct readableBytes value after they are allocated.
Maybe we can simplify these tests.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
|
I'm doing a new review pass. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, thanks !
Sorry, something went wrong.
|
Thanks you @jbonofre! Can we backport this fix to versions like 18.3.0 and release 18.3.1? |
Sorry, something went wrong.
|
@viirya my plan is more to release 19.0.0. Do you really need 18.3.1 ? |
Sorry, something went wrong.
Because we are using 18.3.0 currently, it will be much safer to upgrade with 18.3.1. Is it possible to backport it to 18.3 and release 18.3.1? |
Sorry, something went wrong.
|
@viirya ok, I understand. Let me complete 19.0.0 first, I will prepare 18.3.1 after. |
Sorry, something went wrong.
|
Sorry, something went wrong.
There was a problem hiding this comment.
@Yicong-Huang Should we also fix setReaderAndWriterIndex at BaseVariableWidthVector?
Sorry, something went wrong.
sure will also fix the base! |
Sorry, something went wrong.
Just gently following up on this when you have a chance. Thanks again. |
Sorry, something went wrong.
…offset buffer serialization (#989) ## What's Changed Fix `BaseVariableWidthVector`/`BaseLargeVariableWidthVector` IPC serialization when `valueCount` is 0. ### Problem When `valueCount == 0`, `setReaderAndWriterIndex()` was setting `offsetBuffer.writerIndex(0)`, which means `readableBytes() == 0`. IPC serializer uses `readableBytes()` to determine buffer size, so 0 bytes were written to the IPC stream. This crashes IPC readers in other libraries because Arrow spec requires offset buffer to have at least one entry `[0]`. This is a follow-up to #967 which fixed the same issue in `ListVector`/`LargeListVector`. ### Fix Modify `setReaderAndWriterIndex()` to always use `(valueCount + 1) * OFFSET_WIDTH` for the offset buffer's `writerIndex`, moved outside the if/else branch. When the offset buffer capacity is insufficient (e.g., empty buffer from constructor or loaded via `loadFieldBuffers()`), it reallocates a properly sized buffer on demand. ### Testing Added tests for empty `VarCharVector` and `LargeVarCharVector` verifying offset buffer has correct `readableBytes()` after `setValueCount(0)`. Closes #343 --------- Co-authored-by: Yicong Huang <yicong.huang+data@databricks.com>
Hello @jbonofre, thank you for reviewing and preparing 19.0.0 release. Now as 19.0.0 was released, can you help us prepare 18.3.1 release including these fixes if you have time? Thank you! |
Sorry, something went wrong.
|
We are troubleshooting a regression that ends up in following stack: (java.lang.IndexOutOfBoundsException) readerIndex: 0, writerIndex: 4 (expected: 0 <= readerIndex <= writerIndex <= capacity(0))
io.netty.buffer.AbstractByteBuf.checkIndexBounds():112
io.netty.buffer.AbstractByteBuf.writerIndex():135
io.netty.buffer.NettyArrowBuf.writerIndex():215
io.netty.buffer.NettyArrowBuf.unwrapBuffer():631
Our starting point is this. Is it possible that merged changes break Netty's assumptions in unwrapBuffer? edit: looks like change in #989 was more involved (and more correct?) |
Sorry, something went wrong.
…lized for nested empty arrays (apache#967)" This reverts commit 0f8a080.
| Back | FazBrowse Home | New Git URL |
What's Changed
Fix ListVector/LargeListVector IPC serialization when valueCount is 0.
Problem
When valueCount == 0, setReaderAndWriterIndex() was setting offsetBuffer.writerIndex(0), which means readableBytes() == 0. IPC serializer uses readableBytes() to determine buffer size, so 0 bytes were written to the IPC stream. This crashes IPC readers in other libraries because Arrow spec requires offset buffer to have at least one entry [0].
@viirya:
Fix
Simplify setReaderAndWriterIndex() to always use (valueCount + 1) * OFFSET_WIDTH for offset buffer's writerIndex. When valueCount == 0, this correctly sets writerIndex to OFFSET_WIDTH, ensuring offset[0] is included in serialization.
Testing
Added tests for nested empty lists verifying offset buffer has correct readableBytes().
Closes #343.