andygrove commented on code in PR #1419:
URL: https://github.com/apache/arrow-java/pull/1419#discussion_r4432993801
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -84,15 +84,40 @@ private void importChild(ArrayImporter parent, ArrowArray
src) {
}
private void doImport(ArrowArray.Snapshot snapshot) {
- // A non-zero offset is only meaningful for arrays that have buffers to
offset into.
- // Null arrays carry no buffers, so their offset is inert and safe to
ignore.
- checkState(
- snapshot.offset == 0
- || snapshot.length == 0
- || vector.getField().getType().getTypeID() ==
ArrowType.ArrowTypeID.Null,
- "ArrowArray struct has non-zero offset (%s), which is not supported",
- snapshot.offset);
+ checkState(snapshot.offset >= 0, "ArrowArray struct has negative offset
(%s)", snapshot.offset);
+ if (snapshot.offset != 0 && snapshot.length != 0) {
+ importSliced(snapshot);
+ } else {
+ importUnsliced(snapshot);
+ }
+ }
+
+ /**
+ * Java vectors cannot represent an offset, so the leading {@code offset +
length} values are
+ * imported into a temporary vector and the requested window is then
transferred into the target.
+ */
+ private void importSliced(ArrowArray.Snapshot snapshot) {
+ int offset = checkedCastToInt(snapshot.offset);
+ int length = checkedCastToInt(snapshot.length);
+
+ snapshot.length = Math.addExact(snapshot.offset, snapshot.length);
+ snapshot.offset = 0;
+ // The producer's null count only covers the slice. Without a validity
bitmap the null count
+ // must be 0 for the buffers to load, otherwise it is unknown.
+ boolean hasBitmap = snapshot.n_buffers > 0 &&
MemoryUtil.getLong(snapshot.buffers) != NULL;
Review Comment:
Applied in 63806a6.
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -84,15 +84,40 @@ private void importChild(ArrayImporter parent, ArrowArray
src) {
}
private void doImport(ArrowArray.Snapshot snapshot) {
- // A non-zero offset is only meaningful for arrays that have buffers to
offset into.
- // Null arrays carry no buffers, so their offset is inert and safe to
ignore.
- checkState(
- snapshot.offset == 0
- || snapshot.length == 0
- || vector.getField().getType().getTypeID() ==
ArrowType.ArrowTypeID.Null,
- "ArrowArray struct has non-zero offset (%s), which is not supported",
- snapshot.offset);
+ checkState(snapshot.offset >= 0, "ArrowArray struct has negative offset
(%s)", snapshot.offset);
+ if (snapshot.offset != 0 && snapshot.length != 0) {
+ importSliced(snapshot);
+ } else {
+ importUnsliced(snapshot);
+ }
+ }
+
+ /**
+ * Java vectors cannot represent an offset, so the leading {@code offset +
length} values are
+ * imported into a temporary vector and the requested window is then
transferred into the target.
+ */
+ private void importSliced(ArrowArray.Snapshot snapshot) {
+ int offset = checkedCastToInt(snapshot.offset);
+ int length = checkedCastToInt(snapshot.length);
+
+ snapshot.length = Math.addExact(snapshot.offset, snapshot.length);
+ snapshot.offset = 0;
+ // The producer's null count only covers the slice. Without a validity
bitmap the null count
+ // must be 0 for the buffers to load, otherwise it is unknown.
+ boolean hasBitmap = snapshot.n_buffers > 0 &&
MemoryUtil.getLong(snapshot.buffers) != NULL;
+ snapshot.null_count = hasBitmap ? -1 : 0;
Review Comment:
Fixed in e110114. The importer now advances the buffer pointers past the
leading rows that fill whole bytes of a bitmap, so the temporary vector starts
at most 7 rows before the slice. A bitmap synthesized for it covers at most
`length + 7` rows. The imported buffers shrink the same way, so the target no
longer keeps a wrap of every row before the offset either.
Children whose rows line up with the parent's rows (struct, sparse union,
fixed-size list) get the same skip. They are also trimmed to the slice, because
a sliced struct exports them at full length. Run-end encoded arrays keep the
split-only path, since their children are not indexed by row.
`testImportSliceWithoutValidityBitmapAllocatesOnlyTheSlice` and
`testImportStructSliceWithoutValidityBitmapsAllocatesOnlyTheSlice` import 10
rows at offsets around 65,536 from arrays without bitmaps, and check that the
importing allocator holds less than 1 KB. Before the change it held about 260
KB.
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -84,15 +84,40 @@ private void importChild(ArrayImporter parent, ArrowArray
src) {
}
private void doImport(ArrowArray.Snapshot snapshot) {
- // A non-zero offset is only meaningful for arrays that have buffers to
offset into.
- // Null arrays carry no buffers, so their offset is inert and safe to
ignore.
- checkState(
- snapshot.offset == 0
- || snapshot.length == 0
- || vector.getField().getType().getTypeID() ==
ArrowType.ArrowTypeID.Null,
- "ArrowArray struct has non-zero offset (%s), which is not supported",
- snapshot.offset);
+ checkState(snapshot.offset >= 0, "ArrowArray struct has negative offset
(%s)", snapshot.offset);
+ if (snapshot.offset != 0 && snapshot.length != 0) {
+ importSliced(snapshot);
+ } else {
+ importUnsliced(snapshot);
+ }
+ }
+
+ /**
+ * Java vectors cannot represent an offset, so the leading {@code offset +
length} values are
+ * imported into a temporary vector and the requested window is then
transferred into the target.
+ */
+ private void importSliced(ArrowArray.Snapshot snapshot) {
+ int offset = checkedCastToInt(snapshot.offset);
+ int length = checkedCastToInt(snapshot.length);
+
+ snapshot.length = Math.addExact(snapshot.offset, snapshot.length);
+ snapshot.offset = 0;
+ // The producer's null count only covers the slice. Without a validity
bitmap the null count
+ // must be 0 for the buffers to load, otherwise it is unknown.
+ boolean hasBitmap = snapshot.n_buffers > 0 &&
MemoryUtil.getLong(snapshot.buffers) != NULL;
+ snapshot.null_count = hasBitmap ? -1 : 0;
+
+ try (FieldVector unslicedVector =
vector.getField().createVector(allocator)) {
+ ArrayImporter unslicedImporter =
+ new ArrayImporter(allocator, unslicedVector, dictionaryProvider);
+ unslicedImporter.recursionLevel = recursionLevel;
+ unslicedImporter.underlyingAllocation = underlyingAllocation;
+ unslicedImporter.importUnsliced(snapshot);
+ unslicedVector.makeTransferPair(vector).splitAndTransfer(offset, length);
Review Comment:
Fixed in aead27c and e110114:
- `ExtensionTypeVector.makeTransferPair` now transfers into the underlying
vector of an extension vector target, and `getTo()` still returns the extension
vector. That covers `OpaqueVector` and any other vector that does not override
it, at any depth.
- The importer creates the temporary vector from the imported field when it
has one, which `importVector`, `importVectorSchemaRoot` and
`importIntoVectorSchemaRoot` do. That field has the extension type even when
the vector's `getField()` reports only its storage type, so the temporary
vector has the same classes as the target.
- At the top level the importer transfers between the storage vectors, so
`importIntoVector` into such a vector works as well.
One case is left: `importIntoVector` into a caller's vector that nests such
an extension vector below the top level. No field reports that vector's type
there, so the import now throws an `UnsupportedOperationException` that names
the vector. Before, the struct's transfer pair silently replaced the child: the
target's extension vector became a plain `VarCharVector`.
The tests use a `PlainVector` that overrides neither `getField()` nor
`makeTransferPair()`. They cover it at the top level, in a struct, in a list,
in a sliced record batch, and through `importIntoVector`. `OpaqueVector` is
covered at the top level, in a struct and in a list. `TestExtensionType` tests
the transfer pair itself with `LocationVector`.
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -84,15 +84,40 @@ private void importChild(ArrayImporter parent, ArrowArray
src) {
}
private void doImport(ArrowArray.Snapshot snapshot) {
- // A non-zero offset is only meaningful for arrays that have buffers to
offset into.
- // Null arrays carry no buffers, so their offset is inert and safe to
ignore.
- checkState(
- snapshot.offset == 0
- || snapshot.length == 0
- || vector.getField().getType().getTypeID() ==
ArrowType.ArrowTypeID.Null,
- "ArrowArray struct has non-zero offset (%s), which is not supported",
- snapshot.offset);
+ checkState(snapshot.offset >= 0, "ArrowArray struct has negative offset
(%s)", snapshot.offset);
+ if (snapshot.offset != 0 && snapshot.length != 0) {
+ importSliced(snapshot);
+ } else {
+ importUnsliced(snapshot);
+ }
+ }
+
+ /**
+ * Java vectors cannot represent an offset, so the leading {@code offset +
length} values are
+ * imported into a temporary vector and the requested window is then
transferred into the target.
+ */
+ private void importSliced(ArrowArray.Snapshot snapshot) {
+ int offset = checkedCastToInt(snapshot.offset);
+ int length = checkedCastToInt(snapshot.length);
+
+ snapshot.length = Math.addExact(snapshot.offset, snapshot.length);
+ snapshot.offset = 0;
+ // The producer's null count only covers the slice. Without a validity
bitmap the null count
+ // must be 0 for the buffers to load, otherwise it is unknown.
+ boolean hasBitmap = snapshot.n_buffers > 0 &&
MemoryUtil.getLong(snapshot.buffers) != NULL;
+ snapshot.null_count = hasBitmap ? -1 : 0;
+
+ try (FieldVector unslicedVector =
vector.getField().createVector(allocator)) {
+ ArrayImporter unslicedImporter =
+ new ArrayImporter(allocator, unslicedVector, dictionaryProvider);
+ unslicedImporter.recursionLevel = recursionLevel;
+ unslicedImporter.underlyingAllocation = underlyingAllocation;
+ unslicedImporter.importUnsliced(snapshot);
+ unslicedVector.makeTransferPair(vector).splitAndTransfer(offset, length);
Review Comment:
Fixed both in c8083d5, in `DenseUnionVector.TransferImpl`:
1. The split maps each type id to the position of its child through
`typeMapFields`. `createTransferPairs()` and `transfer()` had the same mix-up,
which put the children of a new target under their positions rather than their
type ids, so those are fixed as well.
2. The split checks whether the values a window selects from each child are
contiguous. Contiguous values are still split without copying. Values with gaps
are copied with `copyValueSafe` and renumbered from 0.
Copying keeps the value count of each child equal to the number of rows that
select it. `DenseUnionVector.setValueCount()` enforces that, and a parent
struct or list calls it after its own split, so keeping the gaps would have
been undone there.
With your example the slice now returns 102 and 107. `TestDenseUnionVector`
covers type ids [5, 10], gaps at the top level and inside a struct, and a
transfer pair that creates its target. `RoundtripTest` imports both cases at
every offset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
