Uh oh!
There was an error while loading. Please reload this page.
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 into
Conversation
Uh oh!
There was an error while loading. Please reload this page.
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
emkornfield
commented
Jan 11, 2020
Sorry I merged the int64 address space change, this will need to be rebased. |
tianchen92
commented
Jan 11, 2020
It doesn't matter., I'll rebase this after this PR merged #6156. |
emkornfield
commented
Feb 24, 2020
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.). |
emkornfield
commented
Apr 29, 2020
Just checking @tianchen92 what is the status of this? |
tianchen92
commented
Apr 29, 2020
It was blocked by #6156 |
emkornfield
commented
Sep 12, 2021
Closing as stale. |
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.