ARROW-5918: [Java] Add get to BaseIntVector interface #4859
Conversation
| Object index = indices.getObject(i); | ||
| if (index != null) { | ||
| int indexAsInt = ((Number) index).intValue(); | ||
| if (!baseIntVector.isNull(i)) { |
There was a problem hiding this comment.
encoded value can never be null?
There was a problem hiding this comment.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
| @@ -23,7 +23,13 @@ | |||
| public interface BaseIntVector extends ValueVector { | |||
|
|
|||
| /** | |||
| * 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); | |||
There was a problem hiding this comment.
Do we also need a setSafe method here?
There was a problem hiding this comment.
Thanks, this method is implemented with setSafe in sub classes :)
There was a problem hiding this comment.
I see. Thanks. I am not sure if we should use separate APIs for set and setSafe methods.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure, thanks, will also add a method like "setUnsafeWithPossibleTruncate"
There was a problem hiding this comment.
Thanks for your efforts, PR updated.
java/vector/src/main/java/org/apache/arrow/vector/BigIntVector.java
Outdated
Show resolved
Hide resolved
java/vector/src/main/java/org/apache/arrow/vector/BigIntVector.java
Outdated
Show resolved
Hide resolved
|
Took a quick pass and LGTM, I think this can be merged once Ci is green. Thanks. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
+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
Related to ARROW-5918.