Repository navigation
Conversation
… a Field NullableStructWriter/SingleStructWriter threw "Unknown type: NULL" when the struct field had a Null-typed child, which broke e.g. StructVector#getTransferPair(Field, ...). Add the NullVector child instead.
|
The "Ensure PR format" check is only waiting for a label. Could a maintainer add |
|
@ump45nose I'm reviewing this PR. I set the label. |
| case NULL: { | ||
| // There is no writer for the Null type, but the child vector still has to exist so that | ||
| // the container matches its field (e.g. when it is created from a Field by a TransferPair). | ||
| FieldType fieldType = new FieldType(addVectorAsNullable, MinorType.NULL.getType(), null, null); | ||
| container.addOrGet(child.getName(), fieldType, NullVector.class); | ||
| break; | ||
| } |
There was a problem hiding this comment.
addVectorAsNullable is lost further down: addOrGet -> FieldType.createNewSingleVector -> MinorType.NULL.getNewVector, which returns new NullVector(field.getName()). That always gives a nullable field with no metadata. So for a child like Field("n", FieldType.notNullable(ArrowType.Null.INSTANCE), null), the struct field doesn't match the input field.
Could we change Types.java (line 167) to return new NullVector(field); and pass the child own FieldType here? That keeps both nullability and metadata. It also fixes Field.createVector, which goes through the same getNewVector.
| case NULL: { | |
| // There is no writer for the Null type, but the child vector still has to exist so that | |
| // the container matches its field (e.g. when it is created from a Field by a TransferPair). | |
| FieldType fieldType = new FieldType(addVectorAsNullable, MinorType.NULL.getType(), null, null); | |
| container.addOrGet(child.getName(), fieldType, NullVector.class); | |
| break; | |
| } | |
| case NULL: | |
| // There is no writer for the Null type, but the child vector still has to exist so that | |
| // the container matches its field (e.g. when it is created from a Field by a TransferPair). | |
| container.addOrGet(child.getName(), child.getFieldType(), NullVector.class); | |
| break; |
|
|
||
| @Test | ||
| public void testStructVectorFromFieldWithNullChild() { | ||
| Field nullField = new Field("null_child", FieldType.nullable(ArrowType.Null.INSTANCE), null); |
There was a problem hiding this comment.
Could this child be non nullable and carry metadata? Then assertEquals(structField, vector.getField()) below checks that the Null child field is kept. As written, it passes even though a non nullable Null child comes back nullable.
| Field nullField = new Field("null_child", FieldType.nullable(ArrowType.Null.INSTANCE), null); | |
| Field nullField = | |
| new Field( | |
| "null_child", | |
| new FieldType(false, ArrowType.Null.INSTANCE, null, Map.of("k", "v")), | |
| null); |
| NullVector toChild = toVector.getChild("null_child", NullVector.class); | ||
| assertNotNull(toChild); | ||
| assertEquals(3, toChild.getValueCount()); | ||
| assertTrue(toChild.isNull(0)); |
There was a problem hiding this comment.
NullVector.isNull always returns true, so this always passes. Checking the child field against the input would be more helpful:
| assertTrue(toChild.isNull(0)); | |
| assertEquals(nullField, toChild.getField()); |
Same idea for line 402: s1 is built through the same getNewVector path, so assertEquals(s1.getField(), toVector.getField()) passes even when both are wrong. Comparing with structField would catch it.
What's Changed
NullableStructWriter/SingleStructWriter(generated fromStructWriters.java) walk the struct field's children in their constructor and create a child vector for each one. There was nocase NULL, so a struct whose field has aNull-typed child threwUnsupportedOperationException: Unknown type: NULL. BecauseStructVectorbuilds its writer as soon as it is constructed, this brokenew StructVector(field, ...)and anything that goes through it, such asStructVector#getTransferPair(Field, BufferAllocator)from the issue.There is no writer for the Null type, so the new case only adds the
NullVectorchild to the container (with the child's nullability). The resulting vector's field now matches the input field, and the other children's writers keep working as before.Tests added to
TestStructVector:testStructVectorTransferPairWithNullType: the reproducer from the issue, plus atransfer()that checks the value count, struct validity and theNullVectorchild.testStructVectorFromFieldWithNullChild: builds aStructVectorstraight from aFieldwith a Null child and an Int child, then writes through the struct writer.Both fail on
mainwithUnknown type: NULLand pass with this change. I also ranTestStructVector,TestComplexWriter,TestListVector,TestMapVector,TestUnionVector,TestDenseUnionVector,TestComplexCopier,TestPromotableWriterandTestSplitAndTransfer(all pass, both the default and netty runs), andmvn -pl vector spotless:check.AI assistance disclosure: this change, its tests and this description were prepared with the help of an AI coding assistant. The tests listed above were run locally against this branch.
Closes #830.