GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors #38261

xxlaykxx · 2023-10-13T12:08:45Z

Rationale for this change

Additionally, a new function getTransferPair(Field, Allocator) is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector.
Added Field.setChildren() method and made children not final - for updating field object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object.

What changes are included in this PR?

getTransferPair method for ComplexType Vectors and for BaseVariableWidthVector's

Are these changes tested?

Yes, some tests have been added to verify these changes.

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs.

Closes: [Java] New getTransferPair which takes Field as parameter for Complex type vectors and BaseVariableWidthVector's #38246

…lex type

github-actions · 2023-10-13T12:09:20Z

⚠️ GitHub issue #38246 has been automatically assigned in GitHub to PR creator.

lidavidm · 2023-10-13T18:18:39Z

java/vector/src/main/java/org/apache/arrow/vector/types/pojo/Field.java

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.

lidavidm · 2023-10-13T18:19:58Z

java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java

+    if (field.getChildren().contains(getDataVector().getField())) {
+      return field;
+    }
+    field.setChildren(Collections.singletonList(getDataVector().getField()));
+    return field;


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).

lidavidm · 2023-10-13T18:21:00Z

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);


Hmm, it's unfortunate that this makes existing callsites ambiguous...we should've renamed it in the original PR.

While there's no docstring for the existing function, let's add a docstring for the new ones at least.

lidavidm · 2023-10-13T18:26:33Z

java/vector/src/test/java/org/apache/arrow/vector/TestVectorSchemaRoot.java

Why was this test changed?

lidavidm · 2023-10-13T18:41:51Z

java/vector/src/main/java/org/apache/arrow/vector/complex/NonNullableStructVector.java

+    field.setChildren(children);
+    return field;


caching the field is OK, but let's not mutate it.

lidavidm · 2023-10-13T18:42:27Z

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);


can't this constructor delegate to the one below?

lidavidm · 2023-10-13T18:42:40Z

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);


delegate this ctor to the one below?

lidavidm · 2023-10-13T18:43:05Z

java/vector/src/main/java/org/apache/arrow/vector/complex/FixedSizeListVector.java

    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);


delegate this ctor to the one below?

xxlaykxx · 2023-10-18T08:18:23Z

@lidavidm could you plz re-review?

xxlaykxx · 2023-10-20T05:24:15Z

@lidavidm still need re-review, plz

conbench-apache-arrow · 2023-10-23T03:36:32Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit a376e3c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 15 possible false positives for unstable benchmarks that are known to sometimes produce them.

…s in a Field type for Complex Type Vectors (apache#38261) (#57) ### Rationale for this change Additionally, a new function **getTransferPair(Field, Allocator)** is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector. Added Field.setChildren() method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### What changes are included in this PR? - `getTransferPair` method for ComplexType Vectors and for BaseVariableWidthVector's - added `Field.setChildren()` method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### Are these changes tested? Yes, some tests have been added to verify these changes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#38246 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: David Li <[email protected]> (cherry picked from commit a376e3c)

…s in a Field type for Complex Type Vectors (apache#38261) ### Rationale for this change Additionally, a new function **getTransferPair(Field, Allocator)** is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector. Added Field.setChildren() method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### What changes are included in this PR? - `getTransferPair` method for ComplexType Vectors and for BaseVariableWidthVector's - added `Field.setChildren()` method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### Are these changes tested? Yes, some tests have been added to verify these changes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#38246 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: David Li <[email protected]>

…s in a Field type for Complex Type Vectors (apache#38261) (#57) ### Rationale for this change Additionally, a new function **getTransferPair(Field, Allocator)** is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector. Added Field.setChildren() method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### What changes are included in this PR? - `getTransferPair` method for ComplexType Vectors and for BaseVariableWidthVector's - added `Field.setChildren()` method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### Are these changes tested? Yes, some tests have been added to verify these changes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#38246 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: David Li <[email protected]> (cherry picked from commit a376e3c)

…s in a Field type for Complex Type Vectors (apache#38261) ### Rationale for this change Additionally, a new function **getTransferPair(Field, Allocator)** is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector. Added Field.setChildren() method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### What changes are included in this PR? - `getTransferPair` method for ComplexType Vectors and for BaseVariableWidthVector's - added `Field.setChildren()` method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### Are these changes tested? Yes, some tests have been added to verify these changes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#38246 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: David Li <[email protected]>

…s in a Field type for Complex Type Vectors (apache#38261) (#57) Additionally, a new function **getTransferPair(Field, Allocator)** is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector. Added Field.setChildren() method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. - `getTransferPair` method for ComplexType Vectors and for BaseVariableWidthVector's - added `Field.setChildren()` method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. Yes, some tests have been added to verify these changes. Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#38246 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: David Li <[email protected]> (cherry picked from commit a376e3c)

…s in a Field type for Complex Type Vectors (apache#38261) ### Rationale for this change Additionally, a new function **getTransferPair(Field, Allocator)** is introduced so that a new Field method is not constructed each time getTransferPair is called on the Vector. Added Field.setChildren() method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### What changes are included in this PR? - `getTransferPair` method for ComplexType Vectors and for BaseVariableWidthVector's - added `Field.setChildren()` method and made **children** not final - for updating **field** object inside complex vector(it possible that Field will be without children on creation) - optimisation for keeping using already existing Field object. ### Are these changes tested? Yes, some tests have been added to verify these changes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#38246 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: David Li <[email protected]>

xxlaykxx added 3 commits October 12, 2023 17:36

apacheGH-38246 added new getTransferPair with Field argument for Comp…

c06ac68

…lex type

apacheGH-38246 small fix

ccf50a5

apacheGH-38246 more improvements and tests

454d196

xxlaykxx requested a review from lidavidm as a code owner October 13, 2023 12:08

github-actions bot added Component: Java awaiting review Awaiting review labels Oct 13, 2023

xxlaykxx and others added 2 commits October 13, 2023 16:01

apacheGH-38246 fixed style issue

996a86c

Merge branch 'apache:main' into apacheGH-38246

5979bf6

lidavidm requested changes Oct 13, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 13, 2023

apacheGH-38246 fixed comments

3f232b3

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 14, 2023

xxlaykxx and others added 2 commits October 14, 2023 12:30

apacheGH-38246 revert unneeded tests changes

62c9899

Merge branch 'apache:main' into apacheGH-38246

cafa892

xxlaykxx requested a review from lidavidm October 14, 2023 14:22

apacheGH-38246 refactoring

2838de2

lidavidm approved these changes Oct 20, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 20, 2023

lidavidm merged commit a376e3c into apache:main Oct 20, 2023
15 of 17 checks passed

lidavidm removed the awaiting merge Awaiting merge label Oct 20, 2023

xxlaykxx mentioned this pull request Oct 23, 2023

GH-38246: [JAVA] added new getTransferPair() function that takes in a… dremio/arrow#57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors #38261

GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors #38261

xxlaykxx commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

lidavidm Oct 13, 2023

xxlaykxx commented Oct 18, 2023

xxlaykxx commented Oct 20, 2023

conbench-apache-arrow bot commented Oct 23, 2023

		@@ -134,8 +134,12 @@ public interface ValueVector extends Closeable, Iterable<ValueVector> {

		TransferPair getTransferPair(String ref, BufferAllocator allocator);

		TransferPair getTransferPair(Field field, BufferAllocator allocator);

GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors #38261

GH-38246: [JAVA] added new getTransferPair() function that takes in a Field type for Complex Type Vectors #38261

Conversation

xxlaykxx commented Oct 13, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Oct 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxlaykxx commented Oct 18, 2023

xxlaykxx commented Oct 20, 2023

conbench-apache-arrow bot commented Oct 23, 2023

xxlaykxx commented Oct 13, 2023 •

edited

Loading