jbonofre commented on code in PR #1419:
URL: https://github.com/apache/arrow-java/pull/1419#discussion_r4432589659
##########
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:
If `snapshot.buffers` is NULL, `MemoryUtil.getLong` read address 0 and
crashes the JVM. The spec only allows NULL when `n_buffers == 0`, so this is a
malformed producer. Still, the unsliced path handles it with an exception
(`NativeUtil.toJavaArray` treats NULL as "no buffers"), and the sliced path
should too:
```suggestion
boolean hasBitmap =
snapshot.n_buffers > 0
&& snapshot.buffers != NULL
&& MemoryUtil.getLong(snapshot.buffers) != NULL;
```
##########
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:
This relies on `makeTransferPair(vector)` + `splitAndTransfer` working for
every vector type, and it doesn't for extension vectors.
`ExtensionTypeVector.makeTransferPair` passes the extension target straight to
the storage vector, which casts it to its own class:
```
java.lang.ClassCastException: class
org.apache.arrow.vector.extension.OpaqueVector cannot be cast to class
org.apache.arrow.vector.VarBinaryVector
```
`UuidVector` (the one in the tests) and `JsonVector` work only because they
override `makeTransferPair`. `OpaqueVector` and any user-defined
`ExtensionTypeVector` that doesn't override it fail, both at the top level and
as a child of a sliced struct or list. A user-defined vector that keeps the
default `getField()` fails as well: `getField()` then returns the storage
field, so the temporary vector is created as the plain storage vector, and that
vector can't transfer into the extension target either.
A sliced `RecordBatch` puts the offset on every column, so any batch with a
custom extension column hits this.
Could we fix it in `ExtensionTypeVector` (unwrap an extension target to its
underlying vector, and make sure the temporary vector has the extension type)
and add tests with a custom extension type that overrides neither `getField()`
nor `makeTransferPair()`?
The alternative is to keep rejecting offsets for these vectors with a clear
error for now.
##########
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:
Producers commonly omit the validity bitmap for columns without nulls. With
`null_count = 0`, `BitVectorHelper.loadValidityBuffer` then allocates and fills
an all-ones bitmap for `offset + length` rows.
For byte-aligned offsets the target keeps a slice of that buffer, so the
whole bitmap stays allocated as long as the target lives.
For example, importing 0 rows at offset 8,000,000 from a non-null `int`
column allocates about 1 MB of bitmap and keeps it after the temporary vector
is closed. That is the main use case of this PR, since a sliced `RecordBatch`
puts the offset on every column.
When a large batch is streamed in slices, the cost of each import grows with
its offset.
The synthesized bitmap should only cover the `length` rows that end up in
the target.
##########
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:
Also, I believe dense unions have two problems in
`DenseUnionVector.TransferImpl.splitAndTransfer`, which this PR now exposes to
C Data imports:
1. `typeCounts` / `typeStarts` are sized by the number of children
(`nextTypeId`) but indexed by the raw type code read from the type buffer. A
dense union with type codes `[5, 10]` and offset 1 fails with
`ArrayIndexOutOfBoundException: Index 10 out of bounds for length 2`. The same
array imports fine with offset 0.
2. It assumes each child's offsets in the window are contiguous: it
transfers `count` child values starting at the first offset it sees and
renumbers them `0..count-1`. The spec only requires the offsets to increase, so
gaps are valid. With one child holding `100..107`, row offsets `[0, 2, 7]` and
a slice of 2 rows at offset 1, the result is `102, 103` instead of `102, 107`,
with no error.
The second one is wrong data with no error, where `main` rejects the same
input today. The tests only use type codes 0 and 1 with contiguous offsets.
Could we either fix the split (map type code to child index, and rebuild the
child offsets instead of assuming they are contiguous), or keep rejecting
offsets for dense unions for now?
--
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]
