-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-15187: [Java] Made reader
initialization lazy and added new getTransferPair()
function that takes in a Field
type
#34424
Changes from 4 commits
bb538c6
ca11887
778b26f
76219f5
aa7e1d7
7f73e3c
fe52705
fa74584
5dcce2e
24deeeb
94fe92d
1ec9a35
b4a0c9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,13 +17,15 @@ | |
|
||
package org.apache.arrow.vector; | ||
|
||
import java.lang.reflect.InvocationTargetException; | ||
import java.util.Collections; | ||
import java.util.Iterator; | ||
|
||
import org.apache.arrow.memory.ArrowBuf; | ||
import org.apache.arrow.memory.BufferAllocator; | ||
import org.apache.arrow.memory.ReferenceManager; | ||
import org.apache.arrow.util.Preconditions; | ||
import org.apache.arrow.vector.complex.reader.FieldReader; | ||
import org.apache.arrow.vector.util.DataSizeRoundingUtil; | ||
import org.apache.arrow.vector.util.TransferPair; | ||
import org.apache.arrow.vector.util.ValueVectorUtility; | ||
|
@@ -50,6 +52,8 @@ public abstract class BaseValueVector implements ValueVector { | |
|
||
protected final BufferAllocator allocator; | ||
|
||
protected volatile FieldReader fieldReader; | ||
|
||
protected BaseValueVector(BufferAllocator allocator) { | ||
this.allocator = Preconditions.checkNotNull(allocator, "allocator cannot be null"); | ||
} | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getReaderImplClass already returns a Class, why do we have to look it up again? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also: why can't getReaderImplClass return a factory function instead of a class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please clarify what you mean by return a factory function? I can change the usage to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't these all be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well returning a Out of curiosity, what is the advantage of returning a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Explicit reflection/metaprogramming isn't necessary here over just a factory. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh I guess the construction would only happen when I call Any chance I could clean that up in a follow up ticket? Or would you prefer I make the changes here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha, looks like we commented the same thing at the same time, :jinx: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather not include reflection here that isn't strictly necessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized we don't need really need the |
||
.newInstance(this); | ||
} catch (ClassNotFoundException | NoSuchMethodException | InstantiationException | IllegalAccessException | | ||
InvocationTargetException e) { | ||
logger.error("Unable to instantiate FieldReader for {} because of: ", getClass().getSimpleName(), e); | ||
throw new RuntimeException(e); | ||
} | ||
} | ||
|
||
return fieldReader; | ||
} | ||
} | ||
|
||
/** | ||
* Container for primitive vectors (1 for the validity bit-mask and one to hold the values). | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be
{@link #getReaderImplClass}
? JavaDoc does not use Markdown.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.