GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type #34424

rtadepalli · 2023-03-03T03:51:52Z

Rationale for this change

This PR closes #15187. FieldReader is being allocated directly in the constructor today, and this PR changes it such that the initialization becomes lazy. 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.

What changes are included in this PR?

Introduce a new getTransferPair method.
Make initializing FieldReader lazy.

Are these changes tested?

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

Are there any user-facing changes?

I am not 100% sure if there are any user facing changes.

There should not be any breaking changes.

Closes: [Java] Allocate the FieldReader for Arrow Vectors only on demand. Also introduce getTransferPair which takes Field as parameter. #15187

…ion that takes in a Field type

github-actions · 2023-03-03T03:52:15Z

Closes: [Java] Allocate the FieldReader for Arrow Vectors only on demand. Also introduce getTransferPair which takes Field as parameter. #15187

github-actions · 2023-03-03T03:52:17Z

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

rtadepalli · 2023-03-03T03:53:27Z

java/vector/src/main/java/org/apache/arrow/vector/BigIntVector.java

@@ -71,7 +73,11 @@ public BigIntVector(String name, FieldType fieldType, BufferAllocator allocator)
   */
  public BigIntVector(Field field, BufferAllocator allocator) {
    super(field, allocator, TYPE_WIDTH);
-    reader = new BigIntReaderImpl(BigIntVector.this);
+    reader = () -> {


This is not thread safe, but I am not sure if adding the Google core libraries dependency to get Suppliers::memoize is allowed. I don't know of other ways to make this thread safe without adding any throws clause.

I tried adding apache.commons.lang3#LazyInitializer but that I think requires that I add throws ConcurrentException to the getReader function, doesn't seem that great.

is it possible to liberate the necessary code from either of these libraries? I'm not sure how much baggage they would drag with them

Thanks for working on this rtadepalli. Instead of the above logic, should we add a new constructor with a new boolean parameter createReader. We create the reader based on the boolean flag. And in the getReader if the reader is null, we throw an exception. Callers who dont need the reader can call the new constructor, and if they try accessing the reader then they get an exception.

@prashanthbdremio I think adding a new constructor would not be backwards compatible, unless we just default the boolean to true in the BigIntVector(field, allocator); constructor (and constructors in other classes). If we do this, I guess we're not really changing much, since the reader will be eagerly created anyways. If you think that is fine, I can make the change.

@lwhite1 I have made some changes to the way the reader is being initialized -- please take another look. Thanks.

rtadepalli · 2023-03-03T03:57:01Z

Right now, this only covers derived classes for BaseFixedWidthVector, since that's what the ticket referenced. Should I make changes to derived classes of BaseVariableWidthVector too?

github-actions · 2023-03-03T04:01:40Z

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

lidavidm · 2023-03-03T14:48:13Z

@lwhite1 or @davisusanibar are you able to take a first look at this?

lidavidm · 2023-03-06T12:45:04Z

Right now, this only covers derived classes for BaseFixedWidthVector, since that's what the ticket referenced. Should I make changes to derived classes of BaseVariableWidthVector too?

Ah, I forgot to reply. I think this is OK. We can make a follow up for the rest of the vector classes.

amol- · 2023-03-30T17:25:26Z

Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍

rtadepalli · 2023-03-30T17:46:42Z

I am not sure I understand -- there are other PRs open against main that have been untouched far longer than this has been. Is there some sort of a cleanup going on? It's still relevant, I just haven't had the chance to work on this because something else came up. I'd appreciate it if this was reopened.

lidavidm · 2023-03-30T17:57:27Z

@rtadepalli sorry about that, some contributors decided to perform some spring cleaning and a few PRs got caught up in that. I reopened the PR.

amol- · 2023-04-13T15:28:28Z

@rtadepalli As you are planning to continue working on this one I converted it to a Draft until you consider it complete. Feel free to tag people when you think it's ready for further review.

rtadepalli

Covered both fixed and variable length vectors here. cc @lidavidm.

rtadepalli · 2023-06-04T20:13:45Z

java/vector/src/main/java/org/apache/arrow/vector/BigIntVector.java

@@ -71,7 +73,11 @@ public BigIntVector(String name, FieldType fieldType, BufferAllocator allocator)
   */
  public BigIntVector(Field field, BufferAllocator allocator) {
    super(field, allocator, TYPE_WIDTH);
-    reader = new BigIntReaderImpl(BigIntVector.this);
+    reader = () -> {


@prashanthbdremio I think adding a new constructor would not be backwards compatible, unless we just default the boolean to true in the BigIntVector(field, allocator); constructor (and constructors in other classes). If we do this, I guess we're not really changing much, since the reader will be eagerly created anyways. If you think that is fine, I can make the change.

@lwhite1 I have made some changes to the way the reader is being initialized -- please take another look. Thanks.

rtadepalli · 2023-06-04T23:43:38Z

java/vector/src/main/java/org/apache/arrow/vector/ExtensionTypeVector.java

@@ -130,6 +130,12 @@ public TransferPair makeTransferPair(ValueVector target) {
    return underlyingVector.makeTransferPair(target);
  }

+  @Override
+  protected Class<? extends FieldReader> getReaderImplClass() {
+    throw new UnsupportedOperationException("Readers for extension types depend on the underlying vector, " +


Throwing an exception here because getReader is delegating to the underlying vector, which to my understanding would always be a subclass of BaseValueVector. This means that there'd be a concrete implementation of getReaderImplClass that'd be available, so delegating to the getReader method of the underlying vector is fine and there's no need of an implementation here.

Please let me know if this is not the case.

Subclasses of ExtensionTypeVector should override this if needed.

That should be fine I think -- should I add a comment explicitly saying that?

Hmm, on second thought: maybe remove the override for getReader here, and put the delegation here instead? That way, users can override this and get the expected behavior. Otherwise they have to override both methods which is a bit of a mess.

lidavidm · 2023-06-13T12:18:25Z

java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java

+      if (fieldReader == null) {
+        try {
+          fieldReader =
+              (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())


getReaderImplClass already returns a Class, why do we have to look it up again?

Also: why can't getReaderImplClass return a factory function instead of a class?

Could you please clarify what you mean by return a factory function? I can change the usage to getReaderImplClass().getInstance if that is preferable. I just used Class.forName since I am more familiar with that option is all.

Can't these all be Function<FieldVector, FieldReader>?

Well returning a FieldReader instance would mean that I'd need to construct it. I want to do that in the synchronized block so I am returning the class type to construct an instance out of.

Out of curiosity, what is the advantage of returning a Function type over the current implementation?

getReaderImplClass can return a Function which you then call.

Explicit reflection/metaprogramming isn't necessary here over just a factory.

Huh I guess the construction would only happen when I call .apply which would in the synchronized block. That does seem nicer...

Any chance I could clean that up in a follow up ticket? Or would you prefer I make the changes here?

Ha, looks like we commented the same thing at the same time, :jinx:

I'd rather not include reflection here that isn't strictly necessary

I realized we don't need really need the ValueVector as an argument to the Function, so I decided to return a Supplier instead.

lidavidm · 2023-06-13T12:19:08Z

java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java

+
+  /**
+   * Default implementation to create a reader for the vector. Depends on the individual vector
+   * class' implementation of `getReaderImpl()` to initialize the reader appropriately.


Should be {@link #getReaderImplClass}? JavaDoc does not use Markdown.

lidavidm

LGTM, though it turns out the supplier is not necessary either (sorry)

lidavidm · 2023-06-16T14:11:27Z

java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java

+    }
+    synchronized (this) {
+      if (fieldReader == null) {
+        fieldReader = getReaderImpl().get();


I'll just point out...if you don't actually need to pass anything, this can just be getReaderInstance() and you can directly return the constructed reader. The whole thing with the class or even the function/supplier isn't necessary to make it lazy.

Sigh... don't know why I didn't realize this last night. Will change.

lidavidm · 2023-06-16T14:12:27Z

java/vector/src/main/java/org/apache/arrow/vector/ExtensionTypeVector.java

@@ -130,6 +130,12 @@ public TransferPair makeTransferPair(ValueVector target) {
    return underlyingVector.makeTransferPair(target);
  }

+  @Override
+  protected Class<? extends FieldReader> getReaderImplClass() {
+    throw new UnsupportedOperationException("Readers for extension types depend on the underlying vector, " +


Subclasses of ExtensionTypeVector should override this if needed.

…ader type when asked for impl

rtadepalli · 2023-06-17T19:18:51Z

@lidavidm ready for another look, thanks.

lidavidm

Thank you! And sorry for all the back-and-forth.

lidavidm · 2023-06-18T15:59:13Z

FYI, if it's ready, can you take this out of draft mode?

rtadepalli · 2023-06-18T16:08:28Z

Oh I thought I didn't have permissions to take it out of draft mode but then I realized that I was looking at this PR while logged out on my phone. Done 👍

conbench-apache-arrow · 2023-06-19T11:05:16Z

Conbench analyzed the 6 benchmark runs on commit c3eaf4cb.

There were 4 benchmark results indicating a performance regression:

Commit Run on ursa-thinkcentre-m75q at 2023-06-18 20:06:38Z
- params=1048576/10000, source=cpp-micro, suite=arrow-acero-aggregate-benchmark
- params=32768/1, source=cpp-micro, suite=arrow-compare-benchmark
and 2 more (see the report linked below)

The full Conbench report has more details.

…w `getTransferPair()` function that takes in a `Field` type (apache#34424) This PR closes apache#15187. `FieldReader` is being allocated directly in the constructor today, and this PR changes it such that the initialization becomes lazy. 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. 1. Introduce a new `getTransferPair` method. 2. Make initializing `FieldReader` lazy. Yes, some tests have been added to verify these changes. I am not 100% sure if there are any user facing changes. There should not be any breaking changes. * Closes: apache#15187 Authored-by: Ramasai <ramasai.tadepalli+3108@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

* apacheGH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type (apache#34424) This PR closes apache#15187. `FieldReader` is being allocated directly in the constructor today, and this PR changes it such that the initialization becomes lazy. 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. 1. Introduce a new `getTransferPair` method. 2. Make initializing `FieldReader` lazy. Yes, some tests have been added to verify these changes. I am not 100% sure if there are any user facing changes. There should not be any breaking changes. * Closes: apache#15187 Authored-by: Ramasai <ramasai.tadepalli+3108@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com> * Fix import * Fix imports --------- Signed-off-by: David Li <li.davidm96@gmail.com> Co-authored-by: rtadepalli <105760760+rtadepalli@users.noreply.github.com>

Made reader initialization lazy and added new getTransferPair() funct…

bb538c6

…ion that takes in a Field type

rtadepalli requested a review from lidavidm as a code owner March 3, 2023 03:51

github-actions bot added Component: Java awaiting review Awaiting review labels Mar 3, 2023

rtadepalli commented Mar 3, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 3, 2023

rtadepalli changed the title ~~GH-15187: [Java] Made reader initialization lazy and added new getTransferPair() function that takes in a Field type~~ GH-15187: [Java] Made reader initialization lazy and added new getTransferPair() function that takes in a Field type Mar 3, 2023

Fix javadoc to add @param annotation for BufferAllocator

ca11887

amol- closed this Mar 30, 2023

lidavidm reopened this Mar 30, 2023

amol- marked this pull request as draft April 13, 2023 15:25

rtadepalli added 2 commits June 4, 2023 15:54

Merge remote-tracking branch 'upstream/main' into apacheGH-15187

778b26f

apacheGH-15187: Make reader instantiation thread safe in BaseValueVector

76219f5

rtadepalli commented Jun 4, 2023

View reviewed changes

rtadepalli requested review from lwhite1 and prashanthbdremio June 5, 2023 00:02

lidavidm reviewed Jun 13, 2023

View reviewed changes

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

apacheGH-15187: Change markdown from devils tick to link

7f73e3c

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

apacheGH-15187: Change class instantiation

fe52705

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 15, 2023

rtadepalli added 2 commits June 15, 2023 22:32

apacheGH-15187: add new lambda to return implementation of the reader

fa74584

Merge remote-tracking branch 'upstream/main' into apacheGH-15187

5dcce2e

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 16, 2023

apacheGH-15187: Fix comments

24deeeb

lidavidm reviewed Jun 16, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 16, 2023

rtadepalli added 2 commits June 17, 2023 15:17

apacheGH-15187: Move supplier and just return new instance of FieldRe…

94fe92d

…ader type when asked for impl

Merge remote-tracking branch 'upstream/main' into apacheGH-15187

1ec9a35

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 17, 2023

apacheGH-15187: remove unneeded Supplier imports

b4a0c9d

lidavidm approved these changes Jun 17, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 17, 2023

rtadepalli marked this pull request as ready for review June 18, 2023 16:08

lidavidm merged commit c3eaf4c into apache:main Jun 18, 2023
16 checks passed

GH-15187: [Java] Made reader initialization lazy and added new getTransferPair() function that takes in a Field type #34424

GH-15187: [Java] Made reader initialization lazy and added new getTransferPair() function that takes in a Field type #34424

Conversation

rtadepalli commented Mar 3, 2023 • edited by github-actions bot

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 Mar 3, 2023

github-actions bot commented Mar 3, 2023

rtadepalli Mar 3, 2023 • edited

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

rtadepalli commented Mar 3, 2023 • edited

github-actions bot commented Mar 3, 2023

lidavidm commented Mar 3, 2023

lidavidm commented Mar 6, 2023

amol- commented Mar 30, 2023

rtadepalli commented Mar 30, 2023

lidavidm commented Mar 30, 2023

amol- commented Apr 13, 2023

rtadepalli left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtadepalli Jun 15, 2023 • edited

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

lidavidm left a 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

rtadepalli commented Jun 17, 2023

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm commented Jun 18, 2023

rtadepalli commented Jun 18, 2023

conbench-apache-arrow bot commented Jun 19, 2023

GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type #34424

GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type #34424

rtadepalli commented Mar 3, 2023 •

edited by github-actions bot

rtadepalli Mar 3, 2023 •

edited

rtadepalli commented Mar 3, 2023 •

edited

rtadepalli Jun 15, 2023 •

edited