ARROW-5918: [Java] Add get to BaseIntVector interface by tianchen92 · Pull Request #4859 · apache/arrow

tianchen92 · 2019-07-12T09:10:01Z

liyafan82 · 2019-07-12T11:48:11Z

java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java

-      Object index = indices.getObject(i);
-      if (index != null) {
-        int indexAsInt = ((Number) index).intValue();
+      if (!baseIntVector.isNull(i)) {


encoded value can never be null?

Nope, see https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java#L72

I see. The benefit of not encoding null is efficient storage. However, the benefit of encoding null is efficient computation (no if branch in the encoding/decoding logic, which makes the CPU pipeline/prefetch more efficient).

We may need some investigation on this. For this particular case, it may be OK to stick to the original assumption.

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

liyafan82 · 2019-07-15T02:47:38Z

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

-   * set value at specific index, note this value may need to be need truncated.
+   * Sets the value at index, note this value may need to be need truncated.
   */
  void setWithPossibleTruncate(int index, long value);


Do we also need a setSafe method here?

Thanks, this method is implemented with setSafe in sub classes :)

I see. Thanks. I am not sure if we should use separate APIs for set and setSafe methods.

Lets update the javadoc to indicate this is the safe version. Unfortuantely, I think this made it into the 0.14.0, so we should try not to rename it. We can always add an unsafe version later.

Sure, thanks, will also add a method like "setUnsafeWithPossibleTruncate"

Thanks for your efforts, PR updated.

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

emkornfield · 2019-07-19T04:13:32Z

Took a quick pass and LGTM, I think this can be merged once Ci is green. Thanks.

codecov-io · 2019-07-19T06:42:22Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4859      +/-   ##
==========================================
+ Coverage   87.42%   89.58%   +2.16%     
==========================================
  Files         994      661     -333     
  Lines      140102    96636   -43466     
  Branches     1418        0    -1418     
==========================================
- Hits       122481    86572   -35909     
+ Misses      17259    10064    -7195     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
python/pyarrow/tests/test_orc.py	`95.45% <0%> (-1.52%)`	⬇️
cpp/src/arrow/util/thread-pool-test.cc	`97.66% <0%> (-0.94%)`	⬇️
cpp/src/parquet/arrow/writer.cc	`96.71% <0%> (-0.04%)`	⬇️
cpp/src/gandiva/precompiled/arithmetic_ops_test.cc	`100% <0%> (ø)`	⬆️
cpp/src/gandiva/function_registry_arithmetic.cc	`100% <0%> (ø)`	⬆️
cpp/src/arrow/type.h	`92.69% <0%> (ø)`	⬆️
cpp/src/arrow/buffer-builder.h	`100% <0%> (ø)`	⬆️
cpp/src/gandiva/precompiled/decimal_ops.cc	`97.17% <0%> (ø)`	⬆️
cpp/src/gandiva/precompiled/decimal_ops_test.cc	`97.43% <0%> (ø)`	⬆️
python/pyarrow/pandas-shim.pxi	`65.26% <0%> (ø)`	⬆️
... and 336 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 c350bba...3732a78. Read the comment docs.

emkornfield · 2019-07-20T05:31:48Z

+1

Related to [ARROW-5918](https://issues.apache.org/jira/browse/ARROW-5918). Author: tianchen <[email protected]> Closes #4859 from tianchen92/ARROW-5918 and squashes the following commits: 3732a78 <tianchen> update javadoc and add unsafe method in BaseIntVector 1562ae1 <tianchen> fix 2 7ddce12 <tianchen> fix 26d30be <tianchen> fix e65473c <tianchen> fix comments fe9c2ec <tianchen> fix 9b8afa6 <tianchen> enable null checking 9f2fc09 <tianchen> fix 5739cc4 <tianchen> ARROW-5918: Add get to BaseIntVector interface

Related to [ARROW-5918](https://issues.apache.org/jira/browse/ARROW-5918). Author: tianchen <[email protected]> Closes apache#4859 from tianchen92/ARROW-5918 and squashes the following commits: 3732a78 <tianchen> update javadoc and add unsafe method in BaseIntVector 1562ae1 <tianchen> fix 2 7ddce12 <tianchen> fix 26d30be <tianchen> fix e65473c <tianchen> fix comments fe9c2ec <tianchen> fix 9b8afa6 <tianchen> enable null checking 9f2fc09 <tianchen> fix 5739cc4 <tianchen> ARROW-5918: Add get to BaseIntVector interface

tianchen92 added 2 commits July 12, 2019 17:07

ARROW-5918: [Java] Add get to BaseIntVector interface

5739cc4

fix

9f2fc09

liyafan82 reviewed Jul 12, 2019

View reviewed changes

fsaintjacques added the Component: Java label Jul 12, 2019

emkornfield reviewed Jul 13, 2019

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/BaseIntVector.java Outdated Show resolved Hide resolved

tianchen92 added 3 commits July 13, 2019 15:28

enable null checking

9b8afa6

fix

fe9c2ec

fix comments

e65473c

liyafan82 reviewed Jul 15, 2019

View reviewed changes

liyafan82 approved these changes Jul 15, 2019

View reviewed changes

emkornfield reviewed Jul 16, 2019

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/BigIntVector.java Outdated Show resolved Hide resolved

fix

26d30be

emkornfield reviewed Jul 18, 2019

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/BigIntVector.java Outdated Show resolved Hide resolved

tianchen92 added 3 commits July 18, 2019 14:50

fix

7ddce12

fix 2

1562ae1

update javadoc and add unsafe method in BaseIntVector

3732a78

emkornfield closed this in 57940e7 Jul 20, 2019

asfimport mentioned this pull request Aug 1, 2019

[Java] Add get to BaseIntVector interface #22328

Closed

Conversation

tianchen92 commented Jul 12, 2019

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!

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!

Uh oh!

Uh oh!

emkornfield commented Jul 19, 2019

Uh oh!

codecov-io commented Jul 19, 2019

Codecov Report

Uh oh!

emkornfield commented Jul 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants