jbonofre commented on code in PR #1419:
URL: https://github.com/apache/arrow-java/pull/1419#discussion_r4451002117
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -84,15 +100,78 @@ 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, /* skippedRows= */ 0, /* trimChildren= */
false);
+ }
+ }
+
+ /**
+ * Java vectors cannot represent an offset, so the rows up to the end of the
slice are imported
+ * into a temporary vector and the slice is then transferred into the
target. Leading rows that
+ * fill whole bytes of a bitmap are skipped by advancing the buffer
pointers, so that the
+ * temporary vector, and any validity bitmap synthesized for it, has fewer
than 8 rows before the
+ * slice.
+ */
+ private void importSliced(ArrowArray.Snapshot snapshot) {
+ // Run-end encoded arrays have no buffers, and their children are not
indexed by row
+ long skippedRows =
+ storageType().getTypeID() == ArrowTypeID.RunEndEncoded ? 0 :
snapshot.offset & ~7L;
+ int offset = checkedCastToInt(snapshot.offset - skippedRows);
+ int length = checkedCastToInt(snapshot.length);
+
+ snapshot.offset = 0;
+ snapshot.length = Math.addExact(offset, length);
+ snapshot.null_count = unknownNullCount(snapshot);
+
+ Field unslicedField = field != null ? field : vector.getField();
Review Comment:
When `field` is null (the public `Data.importIntoVector(allocator, array,
vector, provider)`), the temporary vector is built from `vector.getField()`.
For a dense union, that doesn't reproduce the target.
`DenseUnionVector.getField()` walks `typeFields`, which is indexed by type id,
so it returns the children sorted by type id. The C array and the target keep
the producer's order.
With type ids `[10, 5]` (children `a`, `b`), the temporary vector has
children `[b, a]`. `importUnsliced` loads C child 0 (`a`) into the temporary
type-5 vector, and `createTransferPairs()` then rewrites the target's
`typeMapFields` in the temporary vector's order. If both children are
`IntVector`, `checkTransferable` passes and rows read values from the wrong
child, with no error. If the classes differ, it throws a misleading "its field
create a ..." error.
The same array imports correctly with offset `0`, and through
`Data.importVector`, which passes the field from the C schema. So only the
sliced import without a field is affected. All the dense union tests go through
`importVector`, and `testImportDenseUnionWithTypeIdsWithNonZeroOffset` uses
ascending ids with children of different types, so none of them catch this.
```java
Field field = new Field("u",
FieldType.nullable(new ArrowType.Union(UnionMode.Dense, new int[] {10,
5})),
Arrays.asList(
Field.nullable("a", MinorType.INT.getType()),
Field.nullable("b", MinorType.INT.getType())));
// source rows: a=100, b=201, a=102, b=203
Data.exportVector(allocator, source, null, array);
slice(array, 1, 3);
try (FieldVector target = field.createVector(allocator)) {
Data.importIntoVector(allocator, array, target, null);
// expected [201, 102, 203]
}
```
Could we build the temporary field in the target's child order (children
from `getChildrenFromFields()`, type ids from `typeMapFields`), or make
`DenseUnionVector.getField()` keep the child order?
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -84,15 +100,78 @@ 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, /* skippedRows= */ 0, /* trimChildren= */
false);
+ }
+ }
+
+ /**
+ * Java vectors cannot represent an offset, so the rows up to the end of the
slice are imported
+ * into a temporary vector and the slice is then transferred into the
target. Leading rows that
+ * fill whole bytes of a bitmap are skipped by advancing the buffer
pointers, so that the
+ * temporary vector, and any validity bitmap synthesized for it, has fewer
than 8 rows before the
+ * slice.
+ */
+ private void importSliced(ArrowArray.Snapshot snapshot) {
+ // Run-end encoded arrays have no buffers, and their children are not
indexed by row
+ long skippedRows =
+ storageType().getTypeID() == ArrowTypeID.RunEndEncoded ? 0 :
snapshot.offset & ~7L;
+ int offset = checkedCastToInt(snapshot.offset - skippedRows);
+ int length = checkedCastToInt(snapshot.length);
+
+ snapshot.offset = 0;
+ snapshot.length = Math.addExact(offset, length);
+ snapshot.null_count = unknownNullCount(snapshot);
+
+ Field unslicedField = field != null ? field : vector.getField();
+ try (FieldVector unslicedVector = unslicedField.createVector(allocator)) {
Review Comment:
`Field.createVector` builds a struct with the default `CONFLICT_REPLACE`
policy, so duplicate child names collapse into one.
`importIntoVectorSchemaRoot` uses `StructVector.emptyWithDuplicates` on purpose
for this case (see `testVectorSchemaRootWithDuplicatedFieldNames`), but the
sliced path loses that. With columns `[a, a, b]` and a non-zero offset, the
temporary struct has 2 children, and `importUnsliced` fails with `ArrowArray
struct has 3 children (expected 2)`. The same array imports fine with offset 0.
Keeping the duplicates in the temporary vector is not enough on its own.
`StructTransferPair` maps children by name (`from.getChild(child)`), so both a
columns would be routed to the first one.
One option is to create the temporary struct with the target's conflict
policy and transfer struct children by position. Another is to skip the
temporary vector for structs: the importer already pushes the offset into
struct children, so only the struct's own validity bitmap would need shifting.
##########
vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -669,29 +672,54 @@ public void splitAndTransfer(int startIndex, int length) {
to.reallocOffsetBuffer();
}
- int [] typeCounts = new int[nextTypeId];
- int [] typeStarts = new int[nextTypeId];
- for (int i = 0; i < typeCounts.length; i++) {
- typeCounts[i] = 0;
- typeStarts[i] = -1;
+ int[] childIndexes = new int[typeFields.length];
+ Arrays.fill(childIndexes, -1);
+ for (int i = 0; i < nextTypeId; i++) {
+ childIndexes[typeMapFields[i]] = i;
}
+ // The window selects childCounts[i] values of child i, which are
renumbered from 0. The
+ // offsets of a child only have to increase, so the selected values may
have gaps.
+ int[] childCounts = new int[nextTypeId];
+ int[] childStarts = new int[nextTypeId];
+ int[] childEnds = new int[nextTypeId];
+ boolean[] contiguous = new boolean[nextTypeId];
+ Arrays.fill(contiguous, true);
for (int i = startIndex; i < startIndex + length; i++) {
byte typeId = typeBuffer.getByte(i);
if (typeId >= 0) {
- to.offsetBuffer.setInt((long) (i - startIndex) * OFFSET_WIDTH,
typeCounts[typeId]);
- typeCounts[typeId] += 1;
- if (typeStarts[typeId] == -1) {
- typeStarts[typeId] = offsetBuffer.getInt((long) i * OFFSET_WIDTH);
+ int child = childIndexes[typeId];
+ int offset = offsetBuffer.getInt((long) i * OFFSET_WIDTH);
+ if (childCounts[child] == 0) {
+ childStarts[child] = offset;
+ } else if (offset != childEnds[child]) {
+ contiguous[child] = false;
}
+ childEnds[child] = offset + 1;
+ to.offsetBuffer.setInt((long) (i - startIndex) * OFFSET_WIDTH,
childCounts[child]);
+ childCounts[child] += 1;
}
}
- // transfer vector values
+ // transfer vector values, and copy those of children whose selected
values have gaps
+ for (int i = 0; i < nextTypeId; i++) {
+ if (childCounts[i] > 0 && contiguous[i]) {
+ internalTransferPairs[i].splitAndTransfer(childStarts[i],
childCounts[i]);
+ to.childVectors[typeMapFields[i]] = internalTransferPairs[i].getTo();
+ }
+ }
+ for (int i = startIndex; i < startIndex + length; i++) {
+ byte typeId = typeBuffer.getByte(i);
+ if (typeId >= 0 && !contiguous[childIndexes[typeId]]) {
+ internalTransferPairs[childIndexes[typeId]].copyValueSafe(
Review Comment:
The fallback for children whose offsets have gaps copies values with
`copyValueSafe`, which not every child type supports:
- `RunEndEncodedVector` doesn't override `copyFrom`, so it hits
`BaseValueVector.copyFrom` and throws an `UnsupportedOperationException` with
no message.
- `LargeListVector.copyFrom` throws "LargeListViewVector does not support
copyFrom operation yet".
So `splitAndTransfer` fails for a dense union with a run-end encoded or
large list view child whose offsets in the window are not consecutive, and so
does a sliced C import of one. The exception is thrown after the contiguous
children were already transferred, so the target is left partly filled.
`setValueCount()` recomputes the child value counts from the type buffer, so
I agree the target can't keep the gaps and copying is the way to go.
Could we check up front that every child that needs copying supports it, and
otherwise throw an error that names the child type?
--
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]
