Repository navigation
MINOR: Reject non zero offset on array import - #1263
Merged
Merged
Conversation
robert3005
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
August 19, 2026 13:35
This comment has been minimized.
This comment has been minimized.
jbonofre
requested changes
Aug 27, 2026
Contributor
Author
|
Thanks for the suggestion, I applied it and added test to make sure we don't regress it in the future |
Member
|
@robert3005 thanks! I'm doing a new pass 😄 |
Member
|
By the way, we merged a fix on CI, I will rebase. |
jbonofre
force-pushed
the
rk/importoffset
branch
from
August 30, 2026 06:49
07f209b to
20b5fea
Compare
kou
pushed a commit
that referenced
this pull request
Aug 31, 2026
The `JNI macos-15-intel x86_64` job currently fails on `main` and on every open PR while installing dependencies: ``` ==> Installing node Error: node: no bottle available! This is a Tier 3 configuration: https://docs.brew.sh/Support-Tiers#tier-3 Installing node has failed! `brew bundle` failed! 1 Brewfile dependency failed to install ``` `apache/arrow`'s `cpp/Brewfile` lists `brew "node"`, and Homebrew treats x86_64 macOS as a tier 3 configuration and no longer builds a bottle for it. `brew bundle` doesn't build from source, so the step aborts before anything is built. We don't need Node.js for the JNI build, so this skips installing it via `HOMEBREW_BUNDLE_BREW_SKIP`, the same way #1269 did for `aws-sdk-cpp` and gRPC. Failing runs for reference: * `main`: https://github.com/apache/arrow-java/actions/runs/33283736388/job/99183221397 * PR #1263: https://github.com/apache/arrow-java/actions/runs/33297775855/job/99221106761
Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Robert Kruszewski <github@robertk.io>
jbonofre
force-pushed
the
rk/importoffset
branch
from
August 31, 2026 05:32
20b5fea to
520040f
Compare
Null arrays export no buffers (n_buffers == 0), so the offset has nothing to apply to and never enters the import: doImport short-circuits to an empty buffer list and NullVector.loadFieldBuffers only reads the field node length. Arrow C++ still propagates a slice offset onto the struct for these arrays, so rejecting them regressed imports that previously succeeded, e.g. a sliced all-null column.
Member
|
@robert3005 I updated the I will merge as soon as CI is green. |
jbonofre
approved these changes
Aug 31, 2026
lidavidm
approved these changes
Aug 31, 2026
jbonofre
approved these changes
Aug 31, 2026
jbonofre
added a commit
that referenced
this pull request
Nov 13, 2026
…et (#1419) ## What's Changed The C Data Interface importer ignored the `offset` of an `ArrowArray` struct, and since #1263 rejected it outright. Exporting a sliced `RecordBatch` from C++ puts the slice offset on every child array, so such batches could not be imported into Java. `ArrayImporter` now supports a non-zero offset. Java vectors cannot represent an offset, so the importer loads the rows up to the end of the slice into a temporary vector and transfers the slice into the target with `splitAndTransfer`. Leading rows that fill whole bytes of a bitmap are skipped by advancing the buffer pointers, so the temporary vector, and any validity bitmap synthesized for it, starts fewer than 8 rows before the slice. Children indexed by the rows of their parent (sparse union, fixed-size list) get the same skip and are trimmed to the slice. A struct is imported into the target itself instead, because transferring a struct matches its children by name, which loses children with the same name: its offset is pushed into its children, and only its own validity bitmap is sliced. Run-end encoded arrays are sliced by the split alone, so they fail with a clear error when the rows up to the end of the slice are more than a vector can hold. Fixed-width data buffers and byte-aligned validity bitmaps are shared with the C memory, while offset buffers, unaligned validity bitmaps and view data are copied. The null count reported by a producer only covers the slice, so the temporary import passes an unknown null count (-1) when a validity bitmap is present and 0 when the producer omitted it. The temporary vector is created from the imported field, so it has the extension types of vectors whose `getField()` reports only their storage type, and the top-level transfer happens between storage vectors. Importing into an existing vector fails with a clear error if an extension vector nested in a list, map or union does not report its type. Bugs in the vectors this relies on are fixed: - `ExtensionTypeVector.makeTransferPair` failed with a `ClassCastException` for an extension vector target, unless the subclass overrode it as `UuidVector` and `JsonVector` do and `OpaqueVector` does not. - The `DenseUnionVector` split indexed its per-child state by type id, which failed for type ids other than 0 to n - 1. It also returned wrong values when the offsets of a child had gaps. Such values are now copied before anything is transferred, so a child that does not support copying, such as a run-end encoded vector, fails the split with an error that names it instead of leaving the target partly filled. - `DenseUnionVector.getField()` listed the children in the order of their type ids rather than in their own order. Code that pairs the children of the field with those of the vector by position, such as an exported schema or the importer's temporary vector, mixed up the children of a union whose type ids are not ascending. Tests import 20-row vectors at every offset for int, bit, decimal, decimal256, float16, timestamp with time zone, fixed-size binary, varchar, binary, large varchar, large binary, string and binary views, list, large list, list view, large list view, fixed-size list, map, struct (including a struct of a map and a list of structs), sparse and dense union (including type ids that are not ascending), run-end encoded, extension types (UUID, opaque, and one that overrides neither `getField()` nor `makeTransferPair()`, at the top level and nested) and dictionary encoded arrays, and compare the values and null counts. Others cover a list whose child has an offset of its own, sliced dictionary values, a `VectorSchemaRoot` with duplicate field names, a negative offset or length, and a slice whose offset is beyond the range of an int. They also cover the original report's scenario of a `VectorSchemaRoot` whose children carry the slice offset. Two tests check that importing 10 rows at an offset of 65,536 does not hold memory for the rows before the offset. The documentation now describes offset support and its copying caveat instead of stating that offsets are unsupported. Closes #88. --------- Co-authored-by: JB Onofré <jbonofre@apache.org>
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's Changed
Instead of silently accepting the array and pretending the import went fine reject arrays where offset is non 0. Add mention to the docs that this is not supported
This is documentation and negative case for #251