ARROW-7494: [Java] Remove reader index and writer index from ArrowBuf#6133
ARROW-7494: [Java] Remove reader index and writer index from ArrowBuf#6133tianchen92 wants to merge 5 commits intoapache:masterfrom
Conversation
|
Test failed in Gandiva java test and I don't know how to run Gandiva java test locally, can someone help or give some guidance? thanks! @pravindra @praveenbingo |
| if (valueCount == 0) { | ||
| return valueBuffer.slice(0, 0); | ||
| } | ||
| return valueBuffer.slice(0, typeWidth == 0 ? |
There was a problem hiding this comment.
we should look at whether we're generating extra object in the case that the slice is the same as the original. In that case, we should probably just return the original rather than generate extra objects.
| List<ArrowBuf> result = new ArrayList<>(1); | ||
| setReaderAndWriterIndex(); | ||
| result.add(typeBuffer); | ||
| result.add(sliceTypeBuffer()); |
There was a problem hiding this comment.
calling getFieldBuffers should not create new arrow buf objects. If we're relying on reader/writer index on this we should stop.
| @@ -499,31 +497,25 @@ public void loadFieldBuffers(ArrowFieldNode fieldNode, List<ArrowBuf> ownBuffers | |||
| */ | |||
| public List<ArrowBuf> getFieldBuffers() { | |||
There was a problem hiding this comment.
As above. fieldbuffers should not be causing slices. The fact that we have reader/writer settings here is wrong and we should figure out why it was added. To clarify, getFieldBuffers() is distinct from getBuffers(). The former should be for getting access to underlying data for higher-performance algorithms. The latter is for sending the data over the wire. I think we've mixed up use of both. (Maybe you need to address that in a patch first?)
There was a problem hiding this comment.
Agreed, the main reason is that it uses getFieldBuffers in VectorUnloader which need writer/reader settings, and actually it should be replaced with getBuffers().
I opened a PR for this: #6156
|
Sorry I merged the int64 address space change, this will need to be rebased. |
It doesn't matter., I'll rebase this after this PR merged #6156. |
You need to build the C++ libraries for gandiva and add them to your linked library path (then you should be able to run java unit tests as normal.). |
|
Just checking @tianchen92 what is the status of this? |
It was blocked by #6156 |
github-actions
bot
added
the
needs-rebase
jorgecarleitao
force-pushed
the
master
branch
from
d4608a9 to
356c300
Compare
|
Closing as stale. |
4 participants
Related to ARROW-7494.
Reader and writer index and functionality doesn't belong on a chunk of memory and is due to inheritance from ByteBuf. As part of removing ByteBuf inheritance, we should also remove reader and writer indexes from ArrowBuf functionality. It wastes heap memory for rare utility. In general, a slice can be used instead of a reader/writer index pattern.