Skip to content

ARROW-5476: [Java][Memory] Fix Netty Arrow Buf.#4451

Closed
praveenbingo wants to merge 11 commits intoapache:masterfrom
praveenbingo:ARROW-5476
Closed

ARROW-5476: [Java][Memory] Fix Netty Arrow Buf.#4451
praveenbingo wants to merge 11 commits intoapache:masterfrom
praveenbingo:ARROW-5476

Conversation

@praveenbingo
Copy link
Copy Markdown
Contributor

@praveenbingo praveenbingo commented Jun 2, 2019

  • Fixes the slice to use indexes in netty arrow buf, arrow buffer may not track the right indexes.

@liyafan82
Copy link
Copy Markdown
Contributor

liyafan82 commented Jun 3, 2019

@praveenbingo Thanks for finding this problem. Would you please provide a unit test for it?

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jun 4, 2019 •
edited
Loading

Codecov Report

Merging #4451 into master will increase coverage by 1.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4451      +/-   ##
==========================================
+ Coverage   88.27%   89.47%   +1.19%     
==========================================
  Files         846      645     -201     
  Lines      103662    89845   -13817     
  Branches     1253        0    -1253     
==========================================
- Hits        91512    80392   -11120     
+ Misses      11903     9453    -2450     
+ Partials      247        0     -247
Impacted Files Coverage Δ
r/src/recordbatch.cpp
r/R/Table.R
go/arrow/math/uint64_amd64.go
r/src/symbols.cpp
r/R/Column.R
go/arrow/ipc/file_reader.go
r/src/arrow_types.h
js/src/enum.ts
r/src/array_from_vector.cpp
go/arrow/array/builder.go
... and 191 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37e6183...e8d315d. Read the comment docs.

pravindra
pravindra reviewed Jun 4, 2019
View reviewed changes
java/memory/src/main/java/io/netty/buffer/NettyArrowBuf.java Outdated
Copy link
Copy Markdown
Contributor

@pravindra pravindra Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this creates another dependency on netty in the ReferenceManager, which we want to avoid.

what's wrong with the earlier implementation of getDirectBuffer ?

pravindra
pravindra reviewed Jun 6, 2019
View reviewed changes
java/memory/src/main/java/io/netty/buffer/NettyArrowBuf.java Outdated
Copy link
Copy Markdown
Contributor

@pravindra pravindra Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be 'length - index' ?

Copy link
Copy Markdown
Contributor Author

@praveenbingo praveenbingo Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be..fixed and added a test

@praveenbingo praveenbingo changed the title ARROW-5476: [Java][Memory] Fix Netty Arrow Buf slice. ARROW-5476: [Java][Memory] Fix Netty Arrow Buf. Jun 6, 2019
pravindra
pravindra reviewed Jun 11, 2019
View reviewed changes
java/memory/src/main/java/io/netty/buffer/NettyArrowBuf.java Outdated
Copy link
Copy Markdown
Contributor

@pravindra pravindra Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is different from nioBuffer(index, length) - position is different, limit is different. is that intentional ?

Copy link
Copy Markdown
Contributor Author

@praveenbingo praveenbingo Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the implementation for this API by other ByteBuf classes inside Netty like PooledUnsafeDirectByteBuf. So following the same convention.

Copy link
Copy Markdown
Contributor

@pravindra pravindra Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems indirect - getDirectBuffer already moves the addr by index. position(index) - will effectively move it to 2 * index

Copy link
Copy Markdown
Contributor Author

@praveenbingo praveenbingo Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :( Fixed now.

java/memory/src/main/java/io/netty/buffer/NettyArrowBuf.java Outdated
Copy link
Copy Markdown
Contributor

@pravindra pravindra Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add a comment saying that the returned buffer is zero-positioned ?

Copy link
Copy Markdown
Contributor Author

@praveenbingo praveenbingo Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure will do.

pravindra
pravindra reviewed Jun 11, 2019
View reviewed changes
java/memory/src/test/java/io/netty/buffer/TestNettyArrowBuf.java Outdated
Copy link
Copy Markdown
Contributor

@pravindra pravindra Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you pls add more values ?

for ( intValue : {Integer.MIN_VALUE, Short.MIN_VALUE -1, Short.MIN_VALUE, 0, SHORT.MAX_VALUE, Short.MAX_VALUE + 1, Integer.MAX_VALUE}) { .. }

Copy link
Copy Markdown
Contributor Author

@praveenbingo praveenbingo Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@pravindra pravindra closed this in 8cc6bd4 Jun 11, 2019
@asfimport asfimport mentioned this pull request Jun 11, 2019
Closed
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
@praveenbingo @kou
ARROW-5476: [Java][Memory] Fix Netty Arrow Buf.
1bce23b 
- Fixes the slice to use indexes in netty arrow buf, arrow buffer may not track the right indexes.

Author: Praveen <praveen@dremio.com>

Closes apache#4451 from praveenbingo/ARROW-5476 and squashes the following commits:

9ef785a <Praveen> Fix review comments.
e82c22a <Praveen> Address review comments.
58747f3 <Praveen> Fix lint issues.
60f8d7d <Praveen> Fix more APIs
b2b364b <Praveen> Fixed length of created direct buffer.
6ea540a <Praveen> Remove unused import.
b9d87b3 <Praveen> Add test to ensure nio buffer is positioned correctly.
61b3773 <Praveen> Fix review comments.
3d7579b <Praveen> Fix some more API.
c6344ac <Praveen> Add a test for the slice method.
5724481 <Praveen> ARROW-5476:  Fix Netty Arrow Buf slice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pravindra pravindra pravindra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@praveenbingo @liyafan82 @codecov-io @pravindra