FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-343: Fix ListVector offset buffer not properly serialized for nested empty arrays by Yicong-Huang · Pull Request #967 · apache/arrow-java · GitHub

GH-343: Fix ListVector offset buffer not properly serialized for nested empty arrays - #967

Merged
jbonofre merged 6 commits into
apache:mainfrom
Yicong-Huang:fix/343-empty-nested-list-offset-buffer
Jan 23, 2026
Merged

GH-343: Fix ListVector offset buffer not properly serialized for nested empty arrays#967
jbonofre merged 6 commits into
apache:mainfrom
Yicong-Huang:fix/343-empty-nested-list-offset-buffer

Conversation

Yicong-Huang commented Jan 17, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

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:

The offset buffers are 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:

valueCount=0 → ListVector.setReaderAndWriterIndex() sets offsetBuffer.writerIndex(0)
VectorUnloader.getFieldBuffers() returns the buffer with writerIndex=0
MessageSerializer.writeBatchBuffers() writes the buffer
WriteChannel.write(buffer) checks buffer.readableBytes() which is 0
0 bytes are written to the IPC stream
PyArrow read the batch with the missing buffer → crash when other libraries to read

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.

Yicong-Huang changed the title GH-343 Fix ListVector offset buffer not allocated for nested empty arrays GH-343: Fix ListVector offset buffer not allocated for nested empty arrays Jan 17, 2026

This comment has been minimized.

lidavidm added the bug-fix PRs that fix a big. label Jan 18, 2026
github-actions Bot added this to the 19.0.0 milestone Jan 18, 2026

Copy link
Copy Markdown
Member

@Yicong-Huang can you please rebase the PR ? Thanks !

Yicong-Huang force-pushed the fix/343-empty-nested-list-offset-buffer branch from 8b09237 to 7dbdcc4 Compare January 20, 2026 18:21

viirya commented Jan 21, 2026

Copy link
Copy Markdown
Member

When outer array is empty, nested writers are never invoked, so child list's offset buffer remains unallocated (capacity = 0). This violates Arrow spec which requires offset[0] = 0.

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:

  1. valueCount=0 → ListVector.setReaderAndWriterIndex() sets offsetBuffer.writerIndex(0)
  2. VectorUnloader.getFieldBuffers() returns the buffer with writerIndex=0
  3. MessageSerializer.writeBatchBuffers() writes the buffer
  4. WriteChannel.write(buffer) checks buffer.readableBytes() which is 0
  5. 0 bytes are written to the IPC stream
  6. PyArrow read the batch with the missing buffer → crash when other libraries to read

viirya commented Jan 21, 2026

Copy link
Copy Markdown
Member

Hi @lidavidm @jbonofre, do you think this can catch up the Arrow Java 19.0.0 release?

Yicong-Huang changed the title GH-343: Fix ListVector offset buffer not allocated for nested empty arrays GH-343: Fix ListVector offset buffer not properly serialized for nested empty arrays Jan 21, 2026
validityBuffer.writerIndex(BitVectorHelper.getValidityBufferSizeFromCount(valueCount));
offsetBuffer.writerIndex((valueCount + 1) * OFFSET_WIDTH);
}
validityBuffer.writerIndex(BitVectorHelper.getValidityBufferSizeFromCount(valueCount));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

When valueCount == 0, I think validity buffer writer index should be validityBuffer.writerIndex(0);.

Yicong-Huang Jan 21, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I added the if branch back to handle valueCount==0 case. But I still think it is not necessary?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh, if BitVectorHelper.getValidityBufferSizeFromCount(valueCount) returns 0 for valueCount == 0, then it is okay.

Yicong-Huang requested a review from viirya January 21, 2026 21:27
Comment on lines +1114 to +1116
// Allocate outer only - simulates case where inner is never written to
outerList.allocateNew();
outerList.setValueCount(0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think innerList should also call allocateNew? Not allocated is different to not written.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

added

Comment on lines +1395 to +1397
// Only allocate level0 - simulates case where all nested levels are empty
level0.allocateNew();
level0.setValueCount(0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ditto

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

added!

Yicong-Huang requested a review from viirya January 21, 2026 22:57
Comment on lines +1108 to +1112
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()));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I only have some comments on the tests to simplify them. Otherwise, the fix looks correct to me.

@jbonofre @lidavidm Can you take a look and see if we can get this into the next release?

Thank you!

Copy link
Copy Markdown
Member

I'm doing a new review pass.

jbonofre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks !

jbonofre merged commit 0f8a080 into apache:main Jan 23, 2026
26 checks passed

viirya commented Jan 23, 2026

Copy link
Copy Markdown
Member

Thanks you @jbonofre!

Can we backport this fix to versions like 18.3.0 and release 18.3.1?

Copy link
Copy Markdown
Member

@viirya my plan is more to release 19.0.0. Do you really need 18.3.1 ?

viirya commented Jan 23, 2026

Copy link
Copy Markdown
Member

@viirya my plan is more to release 19.0.0. Do you really need 18.3.1 ?

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?

Copy link
Copy Markdown
Member

@viirya ok, I understand. Let me complete 19.0.0 first, I will prepare 18.3.1 after.

Copy link
Copy Markdown
Contributor Author

@viirya ok, I understand. Let me complete 19.0.0 first, I will prepare 18.3.1 after.

Thanks a lot @viirya @jbonofre!

viirya commented Jan 24, 2026

Copy link
Copy Markdown
Member

@viirya ok, I understand. Let me complete 19.0.0 first, I will prepare 18.3.1 after.

Thank you so much! @jbonofre

viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Yicong-Huang Should we also fix setReaderAndWriterIndex at BaseVariableWidthVector?

Copy link
Copy Markdown
Contributor Author

@Yicong-Huang Should we also fix setReaderAndWriterIndex at BaseVariableWidthVector?

sure will also fix the base!

Copy link
Copy Markdown
Contributor Author

Hi @jbonofre, just wanted to check if there’s any rough ETA for the 19.0.0 or 18.3.1 release. This would help us plan our dependency upgrade. Really appreciate all the work on the releases. Thanks!

cc @viirya

Copy link
Copy Markdown
Contributor Author

Hi @jbonofre, just wanted to check if there’s any rough ETA for the 19.0.0 or 18.3.1 release. This would help us plan our dependency upgrade. Really appreciate all the work on the releases. Thanks!

cc @viirya

Just gently following up on this when you have a chance. Thanks again.

jbonofre pushed a commit that referenced this pull request Mar 12, 2026
…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>

viirya commented Apr 1, 2026

Copy link
Copy Markdown
Member

@viirya ok, I understand. Let me complete 19.0.0 first, I will prepare 18.3.1 after.

Thank you so much! @jbonofre

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!

dmitry-chirkov-dremio commented Jun 18, 2026
edited
Loading

Copy link
Copy Markdown

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?)

lriggs added a commit to lriggs/arrow-java that referenced this pull request Jun 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PRs that fix a big.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++/Java] Error when reading inner lists within a struct in empty outer lists from C++/Python in Java

5 participants


Back | FazBrowse Home | New Git URL