jbonofre commented on code in PR #1253:
URL: https://github.com/apache/arrow-java/pull/1253#discussion_r4192748058
##########
vector/src/main/codegen/templates/ComplexCopier.java:
##########
@@ -187,6 +205,8 @@ private static FieldWriter
getListWriterForReader(FieldReader reader, ListWriter
return (FieldWriter) writer.<#if name ==
"Int">integer<#else>${uncappedName}</#if>();
</#if>
</#list></#list>
+ case FIXEDSIZEBINARY:
Review Comment:
`getListWriterForReader` and `getMapWriterForReader` call
`writer.fixedSizeBinary()` without a width, while the struct path passes
`fixedSizeBinary(name, byteWidth)`.
For maps, `UnionMapWriter.fixedSizeBinary()` delegates to
`entryWriter.fixedSizeBinary(KEY_NAME)` with no width. So copying a `MapVector`
with `FixedSizeBinary` keys or values into an empty target throws, even once
the issue at line 120 is fixed. The list path only works if the target child is
already typed.
Could we pass the width from the reader's `ArrowType.FixedSizeBinary` here
as well? There are no tests for the map path, so one would help.
##########
vector/src/main/codegen/templates/ComplexCopier.java:
##########
@@ -117,6 +117,17 @@ public static void copy(FieldReader reader, FieldWriter
writer) {
writer.writeNull();
}
break;
+ case FIXEDSIZEBINARY:
Review Comment:
`writeFixedSizeBinary(fixedSizeBinaryHolder.buffer)` is the deprecated
overload that carries no `byteWidth`. When the target writer isn't already
typed (a fresh `ListVector.empty(...)`, a union, or an untyped struct field),
`PromotableWriter` falls back to `MinorType.FIXEDSIZEBINARY.getType()` and the
copy throws.
The new test don't catch this because they pre-seed the target child with
`addOrGetVector(FieldType.nullable(new ArrowType.FixedSizeBinary(byteWidth)))`.
Could we set `byteWidth` on the holder and call
`writer.write(fixedSizeBinaryHolder)` instead? `reader.read` already populates
the holder, and `AbstractPromotableFieldWriter.write(FixedSizeBinaryHolder)`
builds the `ArrowType` from it. A test that copies into an untyped target would
be good too.
##########
vector/src/main/codegen/templates/ComplexCopier.java:
##########
@@ -160,6 +171,13 @@ private static FieldWriter
getStructWriterForReader(FieldReader reader, StructWr
</#if>
</#list></#list>
+ case FIXEDSIZEBINARY:
Review Comment:
In the `else` branch, `writer.fixedSizeBinary(name)` is called without a
width. This is reached when the reader's field type is not
`ArrowType.FixedSizeBinary` (for example a union-backed reader), and it then
throws an unhelpful error.
Could we throw an explicit exception with a clear message, or derive the
width from the holder?
--
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]
