lidavidm commented on code in PR #38261:
URL: https://github.com/apache/arrow/pull/38261#discussion_r1358655387
##########
java/vector/src/main/java/org/apache/arrow/vector/types/pojo/Field.java:
##########
Review Comment:
Let's keep this class immutable. A method that makes a copy of the field
with children is OK, but mutating the field isn't.
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java:
##########
@@ -633,7 +663,11 @@ public int getBufferSizeFor(int valueCount) {
@Override
public Field getField() {
- return new Field(getName(), fieldType,
Collections.singletonList(getDataVector().getField()));
+ if (field.getChildren().contains(getDataVector().getField())) {
+ return field;
+ }
+ field.setChildren(Collections.singletonList(getDataVector().getField()));
+ return field;
Review Comment:
Instead of mutating the field here, we can keep the check, but just create
(and cache) a new field if needed. IMO, if the garbage is that much of a
concern, the creator of the vector should provide the expected type up front
(this only exists because a ListVector can set its child type dynamically).
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/FixedSizeListVector.java:
##########
@@ -92,20 +91,43 @@ public FixedSizeListVector(String name,
CallBack unusedSchemaChangeCallback) {
super(allocator);
- this.name = name;
this.validityBuffer = allocator.getEmpty();
this.vector = ZeroVector.INSTANCE;
- this.fieldType = fieldType;
this.listSize = ((ArrowType.FixedSizeList)
fieldType.getType()).getListSize();
Preconditions.checkArgument(listSize >= 0, "list size must be
non-negative");
this.valueCount = 0;
this.validityAllocationSizeInBytes =
getValidityBufferSizeFromCount(INITIAL_VALUE_ALLOCATION);
+ this.field = new Field(name, fieldType, null);
Review Comment:
delegate this ctor to the one below?
##########
java/vector/src/test/java/org/apache/arrow/vector/TestVectorSchemaRoot.java:
##########
Review Comment:
Why was this test changed?
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java:
##########
@@ -95,7 +94,23 @@ public static ListVector empty(String name, BufferAllocator
allocator) {
public ListVector(String name, BufferAllocator allocator, FieldType
fieldType, CallBack callBack) {
super(name, allocator, callBack);
this.validityBuffer = allocator.getEmpty();
- this.fieldType = checkNotNull(fieldType);
+ this.field = new Field(name, fieldType, null);
Review Comment:
delegate this ctor to the one below?
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/NonNullableStructVector.java:
##########
@@ -416,7 +471,8 @@ public Field getField() {
for (ValueVector child : getChildren()) {
children.add(child.getField());
}
- return new Field(name, fieldType, children);
+ field.setChildren(children);
+ return field;
Review Comment:
caching the field is OK, but let's not mutate it.
##########
java/vector/src/main/java/org/apache/arrow/vector/ValueVector.java:
##########
@@ -134,8 +134,12 @@ public interface ValueVector extends Closeable,
Iterable<ValueVector> {
TransferPair getTransferPair(String ref, BufferAllocator allocator);
+ TransferPair getTransferPair(Field field, BufferAllocator allocator);
Review Comment:
Hmm, it's unfortunate that this makes existing callsites ambiguous...we
should've renamed it in the original PR.
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/LargeListVector.java:
##########
@@ -121,9 +119,27 @@ public static LargeListVector empty(String name,
BufferAllocator allocator) {
*/
public LargeListVector(String name, BufferAllocator allocator, FieldType
fieldType, CallBack callBack) {
super(allocator);
- this.name = name;
+ this.field = new Field(name, fieldType, null);
Review Comment:
can't this constructor delegate to the one below?
##########
java/vector/src/main/java/org/apache/arrow/vector/ValueVector.java:
##########
@@ -134,8 +134,12 @@ public interface ValueVector extends Closeable,
Iterable<ValueVector> {
TransferPair getTransferPair(String ref, BufferAllocator allocator);
+ TransferPair getTransferPair(Field field, BufferAllocator allocator);
Review Comment:
While there's no docstring for the existing function, let's add a docstring
for the new ones at least.
--
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]
