Repository navigation
GH-559: [Java] Add FixedSizeBinary support to ComplexCopier - #1253
Maria-Berta wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
Hi @jbonofre @laurentgo @lidavidm @wgtmac — checking in on this PR. |
|
@Maria-Berta I'm doing the review. Thanks! |
| writer.writeNull(); | ||
| } | ||
| break; | ||
| case FIXEDSIZEBINARY: |
There was a problem hiding this 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.
| return (FieldWriter) writer.<#if name == "Int">integer<#else>${uncappedName}</#if>(); | ||
| </#if> | ||
| </#list></#list> | ||
| case FIXEDSIZEBINARY: |
There was a problem hiding this 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.
| </#if> | ||
|
|
||
| </#list></#list> | ||
| case FIXEDSIZEBINARY: |
There was a problem hiding this 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?
Rationale for this change
ComplexCopier did not support copying FixedSizeBinary columns nested in List, Map, Struct, or top-level contexts, throwing UnsupportedOperationException.
What changes are included in this PR?
Adds FIXEDSIZEBINARY cases to:
Are these changes tested?
Yes:
Full TestComplexCopier suite (22 tests) passes with no regressions.
Note: I attempted to add equivalent coverage for Map values of type FixedSizeBinary, but ran into a pre-existing limitation unrelated to this fix — MapWriter/ListWriter's no-arg fixedSizeBinary() delegates to NullableStructWriter.fixedSizeBinary(String), which only looks up an existing child writer and never creates one. The byteWidth-aware overload that does create the vector isn't reachable through the public MapWriter/ListWriter interface. This appears to be a gap in the writer codegen itself rather than something ComplexCopier can work around, so I've left it untested here — happy to open a follow-up issue if that's useful, or take a stab at it if maintainers think it's in scope for this PR.
Closes #559