Fix Arrow array offset bug for sliced arrays by nelson2005 · Pull Request #3 · Goykhman/numbarrow

nelson2005 · 2026-03-31T00:03:13Z

From the fake Slim Shady:

Summary

This PR fixes all adapters to correctly handle Arrow arrays with non-zero offset (sliced/filtered arrays). numbarrow was ignoring the Arrow array offset field, reading from buffer byte 0 regardless — sliced arrays returned wrong data.

What's Fixed

Uniform arrays with offset
Bitmap extraction with offset
Boolean unpacking with offset
String arrays with offset
Date/timestamp arrays (inherited fix)
Struct arrays (inherited fix)

Tests

11 new offset tests covering all adapter types

Performance

Bitmap re-packing vectorized with numpy
Boolean unpacking JIT-compiled

Commits

Add failing tests for Arrow array offset handling
Fix uniform array and bitmap extraction to respect Arrow array offset
Fix BooleanArray adapter to respect Arrow array offset
Fix string array extraction to respect Arrow array offset
Address review feedback: vectorize bitmap, JIT boolean unpacking, test cleanup

…t cleanup

Copilot

Pull request overview

This PR fixes a correctness bug across the NumPy adapters to properly respect non-zero Arrow array offsets (e.g., sliced/filtered arrays), ensuring values and validity bitmaps are read from the correct logical start rather than buffer byte 0.

Changes:

Update fixed-width and boolean adapters to account for pa_array.offset when reading buffers and unpacking bit-packed data.
Update bitmap and string extraction utilities to handle offsets correctly.
Add a new test suite covering offset handling across multiple Arrow array types.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
test/test_offset.py	Adds offset-focused tests across uniform, boolean, datetime, string, struct, and bitmap boundary cases.
numbarrow/utils/arrow_array_utils.py	Applies offset-aware pointer arithmetic for fixed-width arrays, repacks validity bitmaps for offsets, and slices string offsets correctly.
numbarrow/core/is_null.py	Adds a Numba-JIT’d helper to unpack bit-packed boolean buffers with a bit offset.
numbarrow/core/adapters.py	Switches BooleanArray adaptation to use offset-aware boolean unpacking and offset-aware validity bitmap extraction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T00:09:22Z

+    bits = np.unpackbits(raw_bitmap, bitorder="little")
+    sliced_bits = bits[offset:offset + length]
+    pad = (-length) % 8
+    if pad:
+        sliced_bits = np.pad(sliced_bits, (0, pad), mode="constant")
+    result = np.packbits(sliced_bits, bitorder="little")


create_bitmap() repacks sliced bitmaps by calling np.unpackbits(raw_bitmap) on the entire underlying bitmap buffer. For a small slice taken from a very large Arrow array, this can allocate an extremely large temporary bits array (8 * buffer_size) and cause major memory/time overhead. Consider limiting work to only the bytes covering [offset, offset+length) (e.g., slice raw_bitmap[offset//8 : (offset+length+7)//8] and adjust by offset % 8), so repacking scales with the slice length rather than the original buffer size.

From the fake Slim Shady:

Good catch — fixed in 378f7bf. The bitmap repacking now slices raw_bitmap[offset//8 : (offset+length+7)//8] before unpacking, so it scales with slice length rather than the original buffer size.

Copilot · 2026-03-31T00:09:23Z

+    for i in range(n):
+        start = logical_offsets[i]
+        end = logical_offsets[i + 1]
        s = (ctypes.c_char * int(end - start)).from_address(data_p + int(start)).value


create_str_array() builds each string via a ctypes char array and uses .value, which stops at the first NUL byte. Arrow UTF-8 strings can legally contain embedded \x00, so this will truncate those values. Use a length-aware read (e.g., ctypes.string_at(addr, size) or .raw[:size]) and then decode, so strings with embedded NULs round-trip correctly.

Suggested change

s = (ctypes.c_char * int(end - start)).from_address(data_p + int(start)).value

length = int(end - start)

# Use a length-aware read so embedded NUL bytes are preserved.

s_bytes = ctypes.string_at(data_p + int(start), length)

s = s_bytes.decode("utf-8")

From the fake Slim Shady:

Fair point — while this was pre-existing behavior, it's worth fixing alongside the offset work. Fixed in 40b3ae6: switched from .value (stops at first NUL) to ctypes.string_at with explicit length, then decode. Added a test with embedded NUL bytes.

Copilot · 2026-03-31T00:09:23Z

+    if offset == 0:
+        return raw_bitmap
+    # Re-pack bitmap bits starting from the offset bit position
+    num_bytes = (length + 7) // 8
+    if length == 0:
+        return np.zeros(num_bytes, dtype=np.uint8)


Now that create_bitmap() receives length, the offset == 0 fast-path could still return a bitmap view that is much larger than necessary when adapting a short slice starting at 0 (Arrow slices typically keep the original buffers). Consider returning only the minimal required prefix (e.g., raw_bitmap[:(length+7)//8]) so downstream code doesn’t carry around oversized bitmap arrays.

Suggested change

if offset == 0:

return raw_bitmap

# Re-pack bitmap bits starting from the offset bit position

num_bytes = (length + 7) // 8

if length == 0:

return np.zeros(num_bytes, dtype=np.uint8)

# If there are no logical elements, return an empty bitmap view.

if length == 0:

return raw_bitmap[:0]

if offset == 0:

# Fast path: no need to repack bits, just return the minimal prefix

# covering the requested logical length.

num_bytes = (length + 7) // 8

return raw_bitmap[:num_bytes]

# Re-pack bitmap bits starting from the offset bit position

num_bytes = (length + 7) // 8

From the fake Slim Shady:

Good catch — fixed in 378f7bf. The offset==0 fast-path now trims to (length+7)//8 bytes, and the length==0 case is handled first to return an empty view.

Limit unpackbits to the bytes covering [offset, offset+length) instead of the entire bitmap buffer. Also trim the fast-path (offset==0) to return only the minimal prefix covering the requested length.

Use ctypes.string_at with explicit length instead of .value which stops at the first NUL byte. Arrow UTF-8 strings can legally contain embedded NUL characters.

Goykhman · 2026-03-31T11:35:32Z

+
+
+class TestStructOffset:
+    def test_struct_sliced(self):


Can you please add some tests here etc. with slices taken off arrays with None's?

From the fake Slim Shady:

Added in 6855bca — new tests for sliced arrays with nulls in both string (test_string_sliced_with_nulls) and struct (test_struct_sliced_with_nulls) adapters. The struct test covers nulls in different fields at different positions.

Thanks. Will this work?

arr = pa.array([ {"a": 1, "b": 10}, None, {"a": 3, "b": 30}, None, {"a": 5, "b": 50}, ])

From the fake Slim Shady:

Good question — it exposed a gap. The adapter was using StructArray.field() which returns raw child arrays without incorporating the struct's own validity bitmap. So with your example, null struct rows were invisible — they showed up as zeros with no bitmap indication.

Fixed in three commits:

03b4094 — Added is_null_struct(), a @njit helper that checks both the struct-level and field-level bitmaps in a single call

45ff109 — structured_array_adapter() now returns a 3-tuple (struct_bitmap, field_bitmaps, field_datas), extracting the struct validity buffer via buffers()[0]

49d8ef5 — Added your exact example as a test plus a combined test exercising both struct-level and field-level nulls with offset

The two-layer approach is inspired by Awkward Array's BitMaskedArray(RecordArray) design — struct-level and field-level nullability are semantically distinct per the Arrow columnar spec, so we keep them separate rather than AND-ing them eagerly. The is_null_struct() helper composes both layers for @njit consumers.

Goykhman · 2026-04-05T18:18:32Z

+        assert is_null(3, bitmaps["ratio"])
+
+    def test_struct_null_rows_sliced(self):
+        """Goykhman's example: struct array with null rows, sliced."""


Cool but loose the reference please ;)
P.S. See item 25 here.

From the fake Slim Shady:

Ha! Done — removed in 685e596. +10 points reclaimed.

Goykhman · 2026-04-05T21:06:48Z

Thanks for contributing, merging this.

nelson2005 added 5 commits March 30, 2026 20:02

Add failing tests for Arrow array offset handling

ad2aa88

Fix uniform array and bitmap extraction to respect Arrow array offset

a907a2d

Fix BooleanArray adapter to respect Arrow array offset

aeee257

Fix string array extraction to respect Arrow array offset

f7eb957

Address review feedback: vectorize bitmap, JIT boolean unpacking, tes…

8813933

…t cleanup

Copilot AI review requested due to automatic review settings March 31, 2026 00:03

Copilot started reviewing on behalf of nelson2005 March 31, 2026 00:03 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

nelson2005 added 2 commits March 30, 2026 23:11

Optimize bitmap repacking to only process relevant byte range

16b452e

Limit unpackbits to the bytes covering [offset, offset+length) instead of the entire bitmap buffer. Also trim the fast-path (offset==0) to return only the minimal prefix covering the requested length.

Fix string extraction truncating at embedded NUL bytes

585ff1c

Use ctypes.string_at with explicit length instead of .value which stops at the first NUL byte. Arrow UTF-8 strings can legally contain embedded NUL characters.

Goykhman reviewed Mar 31, 2026

View reviewed changes

nelson2005 added 4 commits March 31, 2026 09:01

Add tests for sliced arrays with nulls in string and struct adapters

3a8b90f

Add is_null_struct helper for two-layer struct nullability

03b4094

Return struct-level bitmap from structured_array_adapter

45ff109

Add tests for struct-level null rows with offset

49d8ef5

Goykhman reviewed Apr 5, 2026

View reviewed changes

Remove name reference from test docstring

685e596

Goykhman merged commit c4692e7 into Goykhman:main Apr 5, 2026
3 checks passed

nelson2005 deleted the upstream-fix-arrow-offset branch April 5, 2026 21:13

-        s = (ctypes.c_char * int(end - start)).from_address(data_p + int(start)).value
+        length = int(end - start)
+        # Use a length-aware read so embedded NUL bytes are preserved.
+        s_bytes = ctypes.string_at(data_p + int(start), length)
+        s = s_bytes.decode("utf-8")

Conversation

nelson2005 commented Mar 31, 2026

Summary

What's Fixed

Tests

Performance

Commits

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goykhman Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goykhman commented Apr 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Goykhman Apr 4, 2026 •

edited

Loading