jbonofre commented on code in PR #1419:
URL: https://github.com/apache/arrow-java/pull/1419#discussion_r4464493994
##########
vector/src/main/java/org/apache/arrow/vector/ExtensionTypeVector.java:
##########
@@ -154,8 +154,15 @@ public TransferPair getTransferPair(Field field,
BufferAllocator allocator, Call
return underlyingVector.getTransferPair(field, allocator, callBack);
}
+ /**
+ * Make a transfer pair to {@code target}. An extension vector target
receives the data in its
+ * underlying vector, any other target is passed to the transfer pair of the
underlying vector.
+ */
@Override
public TransferPair makeTransferPair(ValueVector target) {
+ if (target instanceof ExtensionTypeVector) {
Review Comment:
This accepts any `ExtensionTypeVector` as target, without checking that it's
the same extension type.
Before, transferring into a different extension vector failed with a
`ClassCastException` (and `UuidVector` / `JsonVector` still cast to their own
class). Now, when the storage classes match, the data is moved and silently
relabelled: for instance an `OpaqueVector` into another extension vector with
`VarBinary` storage.
What about checking the class, the same way `checkTransferable` does in
`ArrayImporter`:
```java
if (target instanceof ExtensionTypeVector) {
if (!getClass().isInstance(target)) {
throw new IllegalArgumentException(
String.format(
"Cannot transfer %s to %s",
getClass().getSimpleName(), target.getClass().getSimpleName()));
}
return new TransferImpl((ExtensionTypeVector<?>) target);
}
```
##########
vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -518,15 +518,12 @@ public void reset() {
@Override
public Field getField() {
- int childCount = (int) Arrays.stream(typeFields).filter(field -> field !=
null).count();
- List<org.apache.arrow.vector.types.pojo.Field> childFields = new
ArrayList<>(childCount);
- int[] typeIds = new int[childCount];
- for (int i = 0; i < typeFields.length; i++) {
- if (typeFields[i] != null) {
- int curIdx = childFields.size();
- typeIds[curIdx] = i;
- childFields.add(typeFields[i]);
- }
+ // The children keep their order, which is not necessarily the order of
their type ids
+ List<org.apache.arrow.vector.types.pojo.Field> childFields = new
ArrayList<>(nextTypeId);
Review Comment:
`nextTypeId` is a `byte`, so after the 120th `registerNewTypeId()` call (the
spec allows type ids 0..127) it wraps to -128, and `new
ArrayList<>(nextTypeId)` throws `IllegalArgumentException: Illegal Capacity:
-128`.
The previous implementation iterated over `typeFields`, so `getField()`
worked for a union with 128 children. It's a regression, and `getField()` is on
the path of schema export, `VectorSchemaRoot` and the importer.
Could we read the count as assigned, for instance with a small helper:
```java
private int typeCount() {
return Byte.toUnsignedInt(nextTypeId);
}
```
and use it here and for the other `nextTypeId` sizes and loops
(`internalTransferPairs`, `createTransferPairs`, `splitAndTransfer`)? With
-128, the `for (int i = 0; i < nextTypeId; i++)` loops silently do nothing.
Those were already on `main`, but it makes sense to fix them together 😄
--
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]
