ARROW-6117: [Java] Fix the set method of FixedSizeBinaryVector

[liyafan82]

For the set method, if the parameter is null, it should clear the validity bit. However, the current implementation throws a NullPointerException.

[codecov-io]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4995      +/-   ##
==========================================
+ Coverage   87.56%   89.67%    +2.1%     
==========================================
  Files        1003      668     -335     
  Lines      143269    98914   -44355     
  Branches     1418        0    -1418     
==========================================
- Hits       125458    88702   -36756     
+ Misses      17449    10212    -7237     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/arrow/json/converter.cc	90.05% <0%> (-1.76%)	⬇️
cpp/src/arrow/json/chunked-builder.cc	79.91% <0%> (-1.68%)	⬇️
cpp/src/arrow/io/readahead.cc	95.91% <0%> (-1.03%)	⬇️
cpp/src/arrow/util/thread-pool-test.cc	97.66% <0%> (-0.94%)	⬇️
r/src/recordbatch.cpp
r/R/Table.R
js/src/util/fn.ts
go/arrow/array/bufferbuilder.go
r/src/symbols.cpp
rust/datafusion/src/execution/projection.rs
... and 329 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 8558e6c...8b5dd7a. Read the comment docs.

[emkornfield]

while this change seems reasonable to me, there are methods that will ignore null buffers if the intention is not to set them (public void set(int index, int isSet, byte[] value)) CC @pravindra

[pravindra]

I checked the similar APIs in BaseVariableWidthVector.java and VarCharVector.java - the setXX functions do not treat null as a valid argument. So, this change will make the behavior inconsistent with other similar APIs.

https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthVector.java#L967

I'm assuming this was done to avoid the extra branch.

@liyafan82, can you use the set(int index, int isSet, byte[] value) instead for your use-case ?

[liyafan82]

I checked the similar APIs in BaseVariableWidthVector.java and VarCharVector.java - the setXX functions do not treat null as a valid argument. So, this change will make the behavior inconsistent with other similar APIs.

https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthVector.java#L967

I'm assuming this was done to avoid the extra branch.

@liyafan82, can you use the set(int index, int isSet, byte[] value) instead for your use-case ?

@pravindra Thanks a lot for your suggestion.
We should avoid introducing extra branch in this method.

So do you think we should add a parameter check here?

This is slightly different from BaseVariableWidthVector#set: if a null is used there, this illegal argument will be detected by ArrowBuf#setBytes with an error message printed. Here, the illegal argument will not be noticed, until a NullPointerException is produced.

This is trivial. So I am OK whether this check should be added or not.

[emkornfield]

checkNotNull?

[liyafan82]

good suggestion. thanks a lot.

[pravindra]

thanks for fixing this, @liyafan82