You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rtadepalli (via GitHub)" <gi...@apache.org> on 2023/03/03 03:51:55 UTC
[GitHub] [arrow] rtadepalli opened a new pull request, #34424: GH-15187: [Java] Made `reader` initialization lazy and added new getTransferPair() function that takes in a `Field` type
rtadepalli opened a new pull request, #34424:
URL: https://github.com/apache/arrow/pull/34424
### 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?
1. Introduce a new `getTransferPair` method.
2. 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1452929447
:warning: GitHub issue #15187 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1595839161
@lidavidm ready for another look, thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1596193504
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 :+1:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new getTransferPair() function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1124000741
##########
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 = () -> {
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1232386673
##########
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, " +
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1453640581
@lwhite1 or @davisusanibar are you able to take a first look at this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230964898
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230979711
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
Ha, looks like we commented the same thing at the same time, :jinx:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1507173052
@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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] prashanthbdremio commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "prashanthbdremio (via GitHub)" <gi...@apache.org>.
prashanthbdremio commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1131638059
##########
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 = () -> {
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new getTransferPair() function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1452925853
Right now, this only covers derived classes for `BaseFixedWidthVector`, since that's what the ticket referenced. Should I make changes to `BaseVariableWidthVector` too?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1490666104
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new getTransferPair() function that takes in a `Field` type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1452922826
* Closes: #15187
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1228033482
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
getReaderImplClass already returns a Class, why do we have to look it up again?
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
Review Comment:
Should be `{@link #getReaderImplClass}`? JavaDoc does not use Markdown.
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
Also: why can't getReaderImplClass return a factory function instead of a class?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230903645
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
Can't these all be `Function<FieldVector, FieldReader>`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230975739
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230984708
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
I'd rather not include reflection here that isn't strictly necessary
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ramasai1 commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "ramasai1 (via GitHub)" <gi...@apache.org>.
ramasai1 commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230977566
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
Ha, looks like we commented the same thing at the same time :jinx:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1232308217
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,35 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct concrete reader implementation.
+ *
+ * @return Returns a lambda that initializes a reader when called.
+ */
+ protected abstract Supplier<FieldReader> getReaderImpl();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of {@link #getReaderImpl} to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using double-checked locking.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ fieldReader = getReaderImpl().get();
Review Comment:
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.
##########
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, " +
Review Comment:
Subclasses of ExtensionTypeVector should override this if needed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1232325236
##########
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, " +
Review Comment:
That should be fine I think -- should I add a comment explicitly saying that?
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,35 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct concrete reader implementation.
+ *
+ * @return Returns a lambda that initializes a reader when called.
+ */
+ protected abstract Supplier<FieldReader> getReaderImpl();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of {@link #getReaderImpl} to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using double-checked locking.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ fieldReader = getReaderImpl().get();
Review Comment:
Sigh... don't know why I didn't realize this last night. Will change.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230278179
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
Review Comment:
Changed.
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new getTransferPair() function that takes in a `Field` type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1452922843
:warning: GitHub issue #15187 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1490690945
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1231716701
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
I realized we don't need really need the `ValueVector` as an argument to the `Function`, so I decided to return a `Supplier` instead.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #34424:
URL: https://github.com/apache/arrow/pull/34424
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- closed pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
URL: https://github.com/apache/arrow/pull/34424
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230975702
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
`getReaderImplClass` can return a `Function` which you then call.
Explicit reflection/metaprogramming isn't necessary here over just a factory.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new getTransferPair() function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1124000741
##########
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 = () -> {
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1456067415
> 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1596981829
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](http://conbench.ursa.dev/compare/runs/314dba1666594ac5be23b7c3031c59b0...a3df4d4c132441149b94065cdc183cf1/)
- [params=1048576/10000, source=cpp-micro, suite=arrow-acero-aggregate-benchmark](http://conbench.ursa.dev/compare/benchmarks/0648ea38c21d7d13800095b22813075a...0648f63f6d5d701580007963eaab69fe)
- [params=32768/1, source=cpp-micro, suite=arrow-compare-benchmark](http://conbench.ursa.dev/compare/benchmarks/0648ea44990971028000fbd659872d9b...0648f64b50057c05800017b2e67fbfc1)
- and 2 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/14371077761) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1217272625
##########
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, " +
Review Comment:
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.
##########
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 = () -> {
Review Comment:
@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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtadepalli commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "rtadepalli (via GitHub)" <gi...@apache.org>.
rtadepalli commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1230964898
##########
java/vector/src/main/java/org/apache/arrow/vector/BaseValueVector.java:
##########
@@ -143,6 +147,43 @@ long computeCombinedBufferSize(int valueCount, int typeWidth) {
return allocator.getRoundingPolicy().getRoundedSize(bufferSize);
}
+ /**
+ * Each vector has a different reader that implements the FieldReader interface. Overridden methods must make
+ * sure to return the correct type of the reader implementation to instantiate the reader properly.
+ *
+ * @return Returns the implementation class type of the vector's reader.
+ */
+ protected abstract Class<? extends FieldReader> getReaderImplClass();
+
+ /**
+ * Default implementation to create a reader for the vector. Depends on the individual vector
+ * class' implementation of `getReaderImpl()` to initialize the reader appropriately.
+ *
+ * @return Concrete instance of FieldReader by using lazy initialization.
+ */
+ public FieldReader getReader() {
+ FieldReader reader = fieldReader;
+
+ if (reader != null) {
+ return reader;
+ }
+ synchronized (this) {
+ if (fieldReader == null) {
+ try {
+ fieldReader =
+ (FieldReader) Class.forName(getReaderImplClass().getName()).getConstructor(getClass())
Review Comment:
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 was 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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1596191238
FYI, if it's ready, can you take this out of draft mode?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34424:
URL: https://github.com/apache/arrow/pull/34424#issuecomment-1490703830
@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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #34424: GH-15187: [Java] Made `reader` initialization lazy and added new `getTransferPair()` function that takes in a `Field` type
Posted by "lwhite1 (via GitHub)" <gi...@apache.org>.
lwhite1 commented on code in PR #34424:
URL: https://github.com/apache/arrow/pull/34424#discussion_r1131574389
##########
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 = () -> {
Review Comment:
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
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org