ARROW-5726: [Java] Implement a common interface for int vectors #4698

tianchen92 · 2019-06-26T02:57:14Z

Related to ARROW-5726.
Now in DictionaryEncoder#encode it use reflection to pull out the set method and then set values.

Set values by reflection is not efficient and code structure is not elegant such as

Method setter = null;
for (Class<?> c : Arrays.asList(int.class, long.class)) {
try {
setter = indices.getClass().getMethod("setSafe", int.class, c);
break;
} catch (NoSuchMethodException e) {
// ignore
}
}

Implement a common interface for int vectors to directly get set method and set values seems a good choice.

emkornfield · 2019-06-26T08:18:32Z

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

I think it would be better if you made this a long, then down-casted rather then boxing/unboxing.

Another alternative might be to use a visitor pattern, but i'm not sure how efficient those can be made in java.

My bad, the indices from Dictionary is explicitly int type, Object type is not nedded, fixed now. Thanks!

emkornfield · 2019-06-27T05:53:39Z

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

I think this should be technically be long (https://github.com/apache/arrow/blob/master/format/Schema.fbs#L260). Note "Int" is a type which can be 8, 16, 32 or 64 bit.

This method sets the indices(which called dictionary encoded values in DictionaryEncoder) which is explicit int type, see https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java#L55

ah, implementation detail, that seems like it might cause us problems at some point but not ncessary to fix now.

emkornfield · 2019-06-28T05:43:14Z

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

please add asserts or precondition.checkArguments to ensure the value doesn't overflow for all types with smaller bitwidths

emkornfield · 2019-06-28T05:44:53Z

Small improvement on overflow, otherwise looks ok. could you rebase when you've made the change. (I think the CI errors will go away)

tianchen92 · 2019-06-28T07:10:05Z

Small improvement on overflow, otherwise looks ok. could you rebase when you've made the change. (I think the CI errors will go away)

Sure, fixed now.

codecov-io · 2019-06-28T08:42:22Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4698      +/-   ##
==========================================
+ Coverage   86.25%   89.46%    +3.2%     
==========================================
  Files         890      659     -231     
  Lines      131546    95130   -36416     
==========================================
- Hits       113468    85107   -28361     
+ Misses      17726    10023    -7703     
+ Partials      352        0     -352

Impacted Files	Coverage Δ
cpp/src/plasma/thirdparty/ae/ae.c	`70.75% <0%> (-0.95%)`	⬇️
cpp/src/arrow/scalar-test.cc	`100% <0%> (ø)`	⬆️
r/src/recordbatch.cpp
r/R/Table.R
go/arrow/math/uint64_amd64.go
r/src/symbols.cpp
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
go/arrow/ipc/file_reader.go
r/src/arrow_types.h
... and 228 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 27cf503...37ec9cc. Read the comment docs.

emkornfield · 2019-06-29T07:05:18Z

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


+  @Override
+  public void setEncodedValue(int index, int value) {
+    Preconditions.checkArgument(value <= Byte.MAX_VALUE, "value is overflow:" + value);


Suggested change

Preconditions.checkArgument(value <= Byte.MAX_VALUE, "value is overflow:" + value);

Preconditions.checkArgument(value <= Byte.MAX_VALUE, "value is overflow: %s" , value);

Otherwise you pay the cost of string concatenation on every iteration (same applies below)

emkornfield · 2019-06-29T07:07:53Z

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


+  @Override
+  public void setEncodedValue(int index, int value) {
+    Preconditions.checkArgument(value <= 0xFFFF, "value is overflow:" + value);


Suggested change

Preconditions.checkArgument(value <= 0xFFFF, "value is overflow:" + value);

Preconditions.checkArgument(value <= Character.MAX_VALUE, "value is overflow:" + value);

I think this should work here.

emkornfield · 2019-06-29T07:10:17Z

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

+      Object value = vector.getObject(i);
+      if (value != null) { // if it's null leave it null
+        // note: this may fail if value was not included in the dictionary
+        Integer encoded = lookUps.get(value);


as a follow-up to this PR you could implement a Map<Object, int> to avoid the unboxing costs.

Thanks, good suggestion!

emkornfield · 2019-07-01T03:05:19Z

+1, LGTM. Thank you

jacques-n · 2019-07-01T03:21:42Z

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

  }

+  @Override
+  public void setEncodedValue(int index, int value) {


why not do this as long and we can have just a generic setWithPossibleTruncate(int index, long value)? Could be generally useful as this naming/sizing is very dictionary specific.

I think that is a good suggestion and I suggested a long as well. @tianchen92 do you want to open a JIRA/PR to refactor the name/size?

…coder Related to [ARROW-5814](https://issues.apache.org/jira/browse/ARROW-5814). As a follow-up of #4698. Implement a Map<Object, int> for DictionaryEncoder to reduce boxing/unboxing operations. Benchmark: DictionaryEncodeHashMapBenchmarks.testHashMap: avgt 5 31151.345 ± 1661.878 ns/op DictionaryEncodeHashMapBenchmarks.testDictionaryEncodeHashMap: avgt 5 15549.902 ± 771.647 ns/op Author: tianchen <niki.lj@alibaba-inc.com> Closes #4765 from tianchen92/map and squashes the following commits: 38ee5a4 <tianchen> add UT f620033 <tianchen> add javadoc and change package 10596ad <tianchen> fix style 86eb350 <tianchen> add interface 98f4c55 <tianchen> init

Related to [ARROW-5726](https://issues.apache.org/jira/browse/ARROW-5726). Now in DictionaryEncoder#encode it use reflection to pull out the set method and then set values. Set values by reflection is not efficient and code structure is not elegant such as Method setter = null; for (Class<?> c : Arrays.asList(int.class, long.class)) { try { setter = indices.getClass().getMethod("setSafe", int.class, c); break; } catch (NoSuchMethodException e) { // ignore } } Implement a common interface for int vectors to directly get set method and set values seems a good choice. Author: tianchen <niki.lj@alibaba-inc.com> Closes #4698 from tianchen92/ARROW-5726 and squashes the following commits: 37ec9cc <tianchen> resolve comments 0219282 <tianchen> fix overflow b97cbef <tianchen> fix a6f351f <tianchen> resolve comments 5e58c52 <tianchen> Implement a common interface for int vectors

…coder Related to [ARROW-5814](https://issues.apache.org/jira/browse/ARROW-5814). As a follow-up of #4698. Implement a Map<Object, int> for DictionaryEncoder to reduce boxing/unboxing operations. Benchmark: DictionaryEncodeHashMapBenchmarks.testHashMap: avgt 5 31151.345 ± 1661.878 ns/op DictionaryEncodeHashMapBenchmarks.testDictionaryEncodeHashMap: avgt 5 15549.902 ± 771.647 ns/op Author: tianchen <niki.lj@alibaba-inc.com> Closes #4765 from tianchen92/map and squashes the following commits: 38ee5a4 <tianchen> add UT f620033 <tianchen> add javadoc and change package 10596ad <tianchen> fix style 86eb350 <tianchen> add interface 98f4c55 <tianchen> init

Related to [ARROW-5726](https://issues.apache.org/jira/browse/ARROW-5726). Now in DictionaryEncoder#encode it use reflection to pull out the set method and then set values. Set values by reflection is not efficient and code structure is not elegant such as Method setter = null; for (Class<?> c : Arrays.asList(int.class, long.class)) { try { setter = indices.getClass().getMethod("setSafe", int.class, c); break; } catch (NoSuchMethodException e) { // ignore } } Implement a common interface for int vectors to directly get set method and set values seems a good choice. Author: tianchen <niki.lj@alibaba-inc.com> Closes #4698 from tianchen92/ARROW-5726 and squashes the following commits: 37ec9cc <tianchen> resolve comments 0219282 <tianchen> fix overflow b97cbef <tianchen> fix a6f351f <tianchen> resolve comments 5e58c52 <tianchen> Implement a common interface for int vectors

…coder Related to [ARROW-5814](https://issues.apache.org/jira/browse/ARROW-5814). As a follow-up of #4698. Implement a Map<Object, int> for DictionaryEncoder to reduce boxing/unboxing operations. Benchmark: DictionaryEncodeHashMapBenchmarks.testHashMap: avgt 5 31151.345 ± 1661.878 ns/op DictionaryEncodeHashMapBenchmarks.testDictionaryEncodeHashMap: avgt 5 15549.902 ± 771.647 ns/op Author: tianchen <niki.lj@alibaba-inc.com> Closes #4765 from tianchen92/map and squashes the following commits: 38ee5a4 <tianchen> add UT f620033 <tianchen> add javadoc and change package 10596ad <tianchen> fix style 86eb350 <tianchen> add interface 98f4c55 <tianchen> init

Related to [ARROW-5726](https://issues.apache.org/jira/browse/ARROW-5726). Now in DictionaryEncoder#encode it use reflection to pull out the set method and then set values. Set values by reflection is not efficient and code structure is not elegant such as Method setter = null; for (Class<?> c : Arrays.asList(int.class, long.class)) { try { setter = indices.getClass().getMethod("setSafe", int.class, c); break; } catch (NoSuchMethodException e) { // ignore } } Implement a common interface for int vectors to directly get set method and set values seems a good choice. Author: tianchen <niki.lj@alibaba-inc.com> Closes apache#4698 from tianchen92/ARROW-5726 and squashes the following commits: 37ec9cc <tianchen> resolve comments 0219282 <tianchen> fix overflow b97cbef <tianchen> fix a6f351f <tianchen> resolve comments 5e58c52 <tianchen> Implement a common interface for int vectors

…coder Related to [ARROW-5814](https://issues.apache.org/jira/browse/ARROW-5814). As a follow-up of apache#4698. Implement a Map<Object, int> for DictionaryEncoder to reduce boxing/unboxing operations. Benchmark: DictionaryEncodeHashMapBenchmarks.testHashMap: avgt 5 31151.345 ± 1661.878 ns/op DictionaryEncodeHashMapBenchmarks.testDictionaryEncodeHashMap: avgt 5 15549.902 ± 771.647 ns/op Author: tianchen <niki.lj@alibaba-inc.com> Closes apache#4765 from tianchen92/map and squashes the following commits: 38ee5a4 <tianchen> add UT f620033 <tianchen> add javadoc and change package 10596ad <tianchen> fix style 86eb350 <tianchen> add interface 98f4c55 <tianchen> init

emkornfield reviewed Jun 26, 2019

View reviewed changes

emkornfield reviewed Jun 27, 2019

View reviewed changes

emkornfield reviewed Jun 28, 2019

View reviewed changes

tianchen92 added 4 commits June 28, 2019 14:05

Implement a common interface for int vectors

5e58c52

resolve comments

a6f351f

fix

b97cbef

fix overflow

0219282

tianchen92 force-pushed the ARROW-5726 branch from 1e7ccf9 to 0219282 Compare June 28, 2019 07:08

emkornfield reviewed Jun 29, 2019

View reviewed changes

resolve comments

37ec9cc

emkornfield closed this in 91b4cbc Jul 1, 2019

jacques-n reviewed Jul 1, 2019

View reviewed changes

tianchen92 mentioned this pull request Jul 1, 2019

ARROW-5814: [Java] Implement a <Object, int> HashMap for DictionaryEncoder #4765

Closed

asfimport mentioned this pull request Jul 1, 2019

[Java] Implement a common interface for int vectors #22153

Closed

	Preconditions.checkArgument(value <= Byte.MAX_VALUE, "value is overflow:" + value);
	Preconditions.checkArgument(value <= Byte.MAX_VALUE, "value is overflow: %s" , value);

	Preconditions.checkArgument(value <= 0xFFFF, "value is overflow:" + value);
	Preconditions.checkArgument(value <= Character.MAX_VALUE, "value is overflow:" + value);

ARROW-5726: [Java] Implement a common interface for int vectors #4698

ARROW-5726: [Java] Implement a common interface for int vectors #4698

Uh oh!

Conversation

tianchen92 commented Jun 26, 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!

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 28, 2019

Uh oh!

tianchen92 commented Jun 28, 2019

Uh oh!

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

Uh oh!

Codecov Report

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 Jul 1, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jun 28, 2019 •

edited

Loading