ARROW-1837: [Java][Integration] Fix unsigned round trip integration tests #4432

emkornfield · 2019-05-31T08:21:04Z

Show unsigned values can be round-tripped between java and C++
in integration tests. This doesn't fully fix the problem because
the UInt* APIs are mostly wrong because they can't represent the
full range of unsigned values (return types are all too small
because java only has signed types).
While I was at it, I fixed the issue with no batches.

codecov-io · 2019-05-31T09:56:48Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4432      +/-   ##
==========================================
+ Coverage   88.26%   89.46%   +1.19%     
==========================================
  Files         846      645     -201     
  Lines      103357    89543   -13814     
  Branches     1253        0    -1253     
==========================================
- Hits        91233    80110   -11123     
+ Misses      11877     9433    -2444     
+ Partials      247        0     -247

Impacted Files	Coverage Δ
cpp/src/arrow/util/thread-pool-test.cc	`97.66% <0%> (-0.94%)`	⬇️
cpp/src/arrow/type.cc	`95.72% <0%> (-0.24%)`	⬇️
cpp/src/plasma/client.cc	`96.96% <0%> (ø)`	⬆️
cpp/src/arrow/type.h	`94.37% <0%> (ø)`	⬆️
cpp/src/plasma/protocol.cc	`93.76% <0%> (ø)`	⬆️
cpp/src/plasma/test/client_tests.cc	`100% <0%> (ø)`	⬆️
cpp/src/arrow/python/arrow_to_pandas.cc	`91.59% <0%> (ø)`	⬆️
cpp/src/arrow/type_traits.h	`85.71% <0%> (ø)`	⬆️
R/array.R
go/arrow/math/uint64_amd64.go
... and 201 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 40f876b...226c4af. Read the comment docs.

wesm · 2019-05-31T15:21:40Z

Awesome, thanks @emkornfield! @pravindra or @siddharthteotia could you review the Java changes?

wesm · 2019-06-03T15:20:53Z

Rebased. Java reviewers (@jacques-n, @pravindra, or @siddharthteotia), could you take a look? Thanks

praveenbingo

Thanks @emkornfield

praveenbingo · 2019-06-03T17:19:03Z

java/tools/src/main/java/org/apache/arrow/tools/FileToStream.java

is this not different from the behavior earlier - we would have thrown exception earlier for zero batches vs what it looks like proceeding silently now?

Yes, this is intentional. This was discussed on the ML, we think it is reasonable to have a schema without batches.

would it not be better to do that in a different method? I am not sure if existing clients depend on this for validation?

Maybe. Given the fact that there are no unit tests for this code path makes me think this this is only used in integration tests. Are you familiar with any additional use cases? Also this change seems like the correct behavior (I don't see why no batches is an exception) but I suppose it would be better to validate at the file level that there wasn't any corruption (but I don't think there are provisions for this in the spec)

Not sure of exact use cases, but have been dealing with a lot of breaking changes while upgrading dremio to use latest arrow and am wary of anything that looks like change in behavior :)

Agree the new behavior seems like the right one to implement. Sounds good for now.

Understood at some point we should discuss on the ML about API/behavior stability

praveenbingo · 2019-06-03T17:19:28Z

java/tools/src/main/java/org/apache/arrow/tools/StreamToFile.java

praveenbingo · 2019-06-03T17:20:43Z

java/vector/src/main/java/org/apache/arrow/vector/UInt1Vector.java

i am not sure what this means :) why is the method public but not usable externally..

this was copy and paste the real intent was only to use for integration tests. I've updated it.

jacques-n · 2019-06-03T15:39:45Z

java/vector/src/main/java/org/apache/arrow/vector/UInt1Vector.java

why public if this should not be used externally?

this was copy and paste the real intent was only to use for integration tests. I've updated it.

jacques-n · 2019-06-03T15:40:57Z

java/vector/src/main/java/org/apache/arrow/vector/UInt1Vector.java

isn't there some new pattern about these where we optionally check isSet?

that is for the primitive types. getObject is supposed to return null if the bitmap isn't set (and wasn't changed with the PR you are referring to).

jacques-n · 2019-06-03T15:41:20Z

java/vector/src/main/java/org/apache/arrow/vector/UInt2Vector.java

you can return char here. it is an unsigned 2 byte value.

Good point. Removed the method entirely.

Actually just changed it to get

emkornfield · 2019-06-04T04:51:34Z

@jacques-n @praveenbingo I think I responded/fixed all your feedback. Thank you for the review.

- Show unsigned values can be round-tripped between java and C++ in integration tests. This doesn't fully fix the problem because the UInt* APIs are mostly wrong because they can't represent the full range of unsigned values (return types are all too small because java only has signed types). - While I was at it, I fixed the issue with no batches.

emkornfield · 2019-06-04T09:46:43Z

Will fix the python issue later today

praveenbingo

minor comment..looks good otherwise.

praveenbingo · 2019-06-04T10:14:25Z

java/vector/src/main/java/org/apache/arrow/vector/UInt2Vector.java

+   * @param index position of the element.
+   * @return value stored at the index.
+   */
+  public static long get(final ArrowBuf buffer, final int index) {


any reason to not return char and returning a long..

Copy paste bug will fix

praveenbingo

LGTM. Thanks.

wesm · 2019-06-05T02:20:21Z

Thanks everybody

…ests - Show unsigned values can be round-tripped between java and C++ in integration tests. This doesn't fully fix the problem because the UInt* APIs are mostly wrong because they can't represent the full range of unsigned values (return types are all too small because java only has signed types). - While I was at it, I fixed the issue with no batches. Author: Micah Kornfield <emkornfield@gmail.com> Author: emkornfield <emkornfield@gmail.com> Closes apache#4432 from emkornfield/fix_integration_tests and squashes the following commits: 226c4af <Micah Kornfield> fixes 27e4738 <emkornfield> Add missing comma to integration test d8ad3d8 <Micah Kornfield> Address PR feedback a6a23e9 <Micah Kornfield> ARROW-1837: Fix unsigned round trip integration tests

wesm force-pushed the fix_integration_tests branch from dcea791 to fb29381 Compare June 3, 2019 15:20

praveenbingo reviewed Jun 3, 2019

View reviewed changes

jacques-n reviewed Jun 3, 2019

View reviewed changes

emkornfield force-pushed the fix_integration_tests branch from fb29381 to 94f30a2 Compare June 4, 2019 04:49

emkornfield added 2 commits June 3, 2019 21:52

Address PR feedback

d8ad3d8

emkornfield force-pushed the fix_integration_tests branch from 94f30a2 to d8ad3d8 Compare June 4, 2019 04:53

Add missing comma to integration test

27e4738

praveenbingo approved these changes Jun 4, 2019

View reviewed changes

fixes

226c4af

praveenbingo approved these changes Jun 4, 2019

View reviewed changes

wesm closed this in 746805c Jun 5, 2019

asfimport mentioned this pull request Jun 5, 2019

[Java] Unable to read unsigned integers outside signed range for bit width in integration tests #17831

Closed

ARROW-1837: [Java][Integration] Fix unsigned round trip integration tests #4432

ARROW-1837: [Java][Integration] Fix unsigned round trip integration tests #4432

Uh oh!

Conversation

emkornfield commented May 31, 2019

Uh oh!

codecov-io commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wesm commented May 31, 2019

Uh oh!

wesm commented Jun 3, 2019

Uh oh!

praveenbingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Jun 4, 2019

Uh oh!

emkornfield commented Jun 4, 2019

Uh oh!

praveenbingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praveenbingo left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Jun 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented May 31, 2019 •

edited

Loading