jbonofre commented on code in PR #1379:
URL: https://github.com/apache/arrow-java/pull/1379#discussion_r4489380888
##########
vector/src/test/java/org/apache/arrow/vector/TestStructVector.java:
##########
@@ -379,6 +381,63 @@ public void
testStructVectorTransferPairWithExtensionType() {
toVector.close();
}
+ @Test
+ public void testStructVectorTransferPairWithNullType() {
+ Field nullField = new Field("null_child",
FieldType.nullable(ArrowType.Null.INSTANCE), null);
+ Field structField =
+ new Field("struct", FieldType.nullable(new ArrowType.Struct()),
List.of(nullField));
+
+ try (StructVector s1 = (StructVector) structField.createVector(allocator))
{
+ NullVector nullVector =
+ s1.addOrGet("null_child",
FieldType.nullable(ArrowType.Null.INSTANCE), NullVector.class);
+ s1.allocateNew();
+ s1.setIndexDefined(0);
+ s1.setNull(1);
+ s1.setIndexDefined(2);
+ nullVector.setValueCount(3);
+ s1.setValueCount(3);
+
+ TransferPair tp = s1.getTransferPair(structField, allocator);
+ try (StructVector toVector = (StructVector) tp.getTo()) {
+ assertEquals(s1.getField(), toVector.getField());
+
+ tp.transfer();
+ assertEquals(3, toVector.getValueCount());
+ assertFalse(toVector.isNull(0));
+ assertTrue(toVector.isNull(1));
+ assertFalse(toVector.isNull(2));
+ NullVector toChild = toVector.getChild("null_child", NullVector.class);
+ assertNotNull(toChild);
+ assertEquals(3, toChild.getValueCount());
+ assertTrue(toChild.isNull(0));
+ }
+ }
+ }
+
+ @Test
+ public void testStructVectorFromFieldWithNullChild() {
+ Field nullField = new Field("null_child",
FieldType.nullable(ArrowType.Null.INSTANCE), null);
Review 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.
```suggestion
Field nullField =
new Field(
"null_child",
new FieldType(false, ArrowType.Null.INSTANCE, null, Map.of("k",
"v")),
null);
```
##########
vector/src/main/codegen/templates/StructWriters.java:
##########
@@ -86,6 +86,13 @@ public class ${mode}StructWriter extends AbstractFieldWriter
{
case EXTENSIONTYPE:
extension(child.getName(), child.getType());
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).
+ FieldType fieldType = new FieldType(addVectorAsNullable,
MinorType.NULL.getType(), null, null);
+ container.addOrGet(child.getName(), fieldType, NullVector.class);
+ break;
+ }
Review 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`.
```suggestion
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;
```
##########
vector/src/test/java/org/apache/arrow/vector/TestStructVector.java:
##########
@@ -379,6 +381,63 @@ public void
testStructVectorTransferPairWithExtensionType() {
toVector.close();
}
+ @Test
+ public void testStructVectorTransferPairWithNullType() {
+ Field nullField = new Field("null_child",
FieldType.nullable(ArrowType.Null.INSTANCE), null);
+ Field structField =
+ new Field("struct", FieldType.nullable(new ArrowType.Struct()),
List.of(nullField));
+
+ try (StructVector s1 = (StructVector) structField.createVector(allocator))
{
+ NullVector nullVector =
+ s1.addOrGet("null_child",
FieldType.nullable(ArrowType.Null.INSTANCE), NullVector.class);
+ s1.allocateNew();
+ s1.setIndexDefined(0);
+ s1.setNull(1);
+ s1.setIndexDefined(2);
+ nullVector.setValueCount(3);
+ s1.setValueCount(3);
+
+ TransferPair tp = s1.getTransferPair(structField, allocator);
+ try (StructVector toVector = (StructVector) tp.getTo()) {
+ assertEquals(s1.getField(), toVector.getField());
+
+ tp.transfer();
+ assertEquals(3, toVector.getValueCount());
+ assertFalse(toVector.isNull(0));
+ assertTrue(toVector.isNull(1));
+ assertFalse(toVector.isNull(2));
+ NullVector toChild = toVector.getChild("null_child", NullVector.class);
+ assertNotNull(toChild);
+ assertEquals(3, toChild.getValueCount());
+ assertTrue(toChild.isNull(0));
Review Comment:
`NullVector.isNull` always returns true, so this always passes. Checking the
child field against the input would be more helpful:
```suggestion
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.
--
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]
