ARROW-6022: [Java] Support equals API in ValueVector to compare two vectors equal #4933

tianchen92 · 2019-07-24T10:38:08Z

In some case, this feature is useful.
In ARROW-1184, Dictionary#equals not work due to the lack of this API.
Moreover, we already implemented equals(int index, ValueVector target, int targetIndex), so this new added API could reuse it.

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

emkornfield · 2019-07-26T05:06:38Z

CC @BryanCutler who I think reviewed the dictionary.equals.

codecov-io · 2019-07-29T08:45:46Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4933      +/-   ##
==========================================
+ Coverage   87.57%   89.68%   +2.11%     
==========================================
  Files        1008      670     -338     
  Lines      143790    99342   -44448     
  Branches     1418        0    -1418     
==========================================
- Hits       125924    89095   -36829     
+ Misses      17504    10247    -7257     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/arrow/csv/column-builder.cc	`95.29% <0%> (-1.77%)`	⬇️
cpp/src/plasma/thirdparty/ae/ae.c	`70.75% <0%> (-0.95%)`	⬇️
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
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
... and 333 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 229fe33...a4e9c48. Read the comment docs.

tianchen92 · 2019-07-31T03:02:35Z

@emkornfield @pravindra Thanks a lot for your comments, I updated this PR, and would you and @BryanCutler mind take a look?

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

pravindra · 2019-08-01T06:49:22Z

the change lgtm. @BryanCutler, can you please take a look too - especially the checks for the complex type vectors ?

tianchen92 · 2019-08-02T23:21:29Z

@BryanCutler Mind taking a look when you are available? thanks

java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java

java/vector/src/main/java/org/apache/arrow/vector/complex/NonNullableStructVector.java

pravindra · 2019-08-05T07:37:52Z

java/vector/src/main/java/org/apache/arrow/vector/complex/NonNullableStructVector.java

StructVector extends from this one. it should compare the validity bits too.

Thanks, fixed.

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java

pravindra · 2019-08-05T08:12:31Z

dumb question - does it make sense to move the equals/hash code out of the value vectors and use visitors like cpp ? I think that'll give more flexibility to do custom stuff (eg. compare approx for floating point, skip compares for names, ..)

tianchen92 · 2019-08-05T08:32:51Z

dumb question - does it make sense to move the equals/hash code out of the value vectors and use visitors like cpp ? I think that'll give more flexibility to do custom stuff (eg. compare approx for floating point, skip compares for names, ..)

Thanks for your comments. I am not sure, if it makes sense, I would like to do it as a follow-up in a seperate issue. What do you think on this proposal? @emkornfield

pravindra · 2019-08-08T07:32:19Z

my primary concern is that the current implementation is not very efficient since the "vector equals" (i.e vv->equals()) is computed by iterating over each element in the vector (vv->equals(i, other, i). This means -

the type/child checks are repeated for each element (for a 4K batch size, the check is done 4K times instead of once)
we are not able to do optimisations at the vector level by first comparing the bitmaps (i.e if the bitmaps don't match, the vectors can't be equal).

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

tianchen92 · 2019-08-08T09:12:45Z

my primary concern is that the current implementation is not very efficient since the "vector equals" (i.e vv->equals()) is computed by iterating over each element in the vector (vv->equals(i, other, i). This means -

the type/child checks are repeated for each element (for a 4K batch size, the check is done 4K times instead of once)

we are not able to do optimisations at the vector level by first comparing the bitmaps (i.e if the bitmaps don't match, the vectors can't be equal).

Thanks for pointing, you are right, do you mean we implement something like this?

public class VectorEqualsVisitor {
protected final ValueVector right;
public VectorEqualsVisitor(ValueVector right) {
this.right = right;
}
private boolean compareBaseFixedWidthVectors(BaseFixedWidthVector left) {}
private boolean compareBaseVariableWidthVectors(BaseVariableWidthVector left) {}
public boolean visit(BaseFixedWidthVector left) {
return compareBaseFixedWidthVectors(left);
}
public boolean visit(BaseVariableWidthVector left) {
return compareBaseVariableWidthVectors(left);
}
......
}

In ValueVector,
equals(VectorEqualsVisitor visitor) {
return visitor.visit(this);
}

pravindra · 2019-08-08T10:04:17Z

@tianchen92 - yes, I think with the approach you pointed out, we could do the type checking just once per vector and make the vector-compare/range-compare efficient.

tianchen92 · 2019-08-08T15:08:10Z

@tianchen92 - yes, I think with the approach you pointed out, we could do the type checking just once per vector and make the vector-compare/range-compare efficient.

I updated this PR, please help take a look, thanks :)

praveenbingo · 2019-08-08T17:21:05Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

we can have fast paths for all values being non-null, but they can be addressed in follow up patches..

Sure, thanks for your comments :)

BryanCutler

Looks pretty good in general, thanks for doing this @tianchen92 ! I have a couple questions:

There seems to be some vectors that implement equals and the same code is also in RangeEqualsVisitor. Is this planned to be merged?
It's a little confusing to have both visitor and equals APIs for the same thing too. Should we keep the visitor API private?
It looks like we check names of child vectors for complex types only, and not the actual FieldVector name. Is that consistent with other languages like C++? I think I saw some discussion on this, but want to make sure

BryanCutler · 2019-08-08T17:24:30Z

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

Can you add a unit test to check if ZeroVector == ZeroVector?

Sure, done.

BryanCutler · 2019-08-08T17:25:33Z

java/vector/src/test/java/org/apache/arrow/vector/types/pojo/TestExtensionType.java

Can you move this to the ExtensionTypeVector class?

BryanCutler · 2019-08-08T17:51:03Z

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

What would happen if index or toIndex is out of range? Can you have a unit test for this?

Good catch, there will be some problems, if the index > valueCount it won't throw exception which is not right. I added index check in this method.

BryanCutler · 2019-08-08T17:53:47Z

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

I'm not sure why we have equals that checks a range of values and accpet a RangeEqualsVisitor? That seems redundant

Equals(int index, ValueVector vector, int toIndex) compares single value and RangeEqualsVisitor compares a range of values.

I must have missed the discussion when equals was added to compare a single value. It doesn't seem like that would be very useful to me, was there a specific use case to support this?

Yes, we did a refactor for dictionary encoding few weeks ago, hashCode/equals API are used for DictionaryHashTable (https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryHashTable.java#L138) to avoid memory copy introduced by previous implementation.
PR is here #4846.

agree with @BryanCutler - this causes duplication of code/tests. Is there a reason to not use the RangeEquals API (with range = 1) instead ?

I'm fine if you want to do this in a different PR.

@pravindra Ah, I have already made it reuse RangeEquals API(range ==1).

Sorry, I meant can we remove the equals() API from ValueVector, and it's callers be modified to instead use the RangeEquals() API ? I think keeping the minimal interface in ValueVector is ideal.

I see, right, could be done in a different PR.

BryanCutler · 2019-08-08T17:57:56Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

This looks like a duplicate of what is already in BaseFixedWidthVector.equals, can you consolidate?

fixed, removed to CompareUtility for reuse.

BryanCutler · 2019-08-08T19:21:51Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

better yet, you don't even need to build the left/rightChildren lists. just loop over children names here and make a visitor new RangeEqualsVisitor(rightVector.getChild(child), leftStart, rightStart, length) and call left.getChild(child).accept(visitor)`

Right, fixed, thanks!

BryanCutler · 2019-08-08T19:24:14Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

could you rename these start -> startByteLeft etc instead of 1/2?

also with the instances below

Thanks, fixed.

BryanCutler · 2019-08-08T19:25:49Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

please don't use 1 and 2 in variable names..

Thanks, fixed.

BryanCutler · 2019-08-08T19:26:19Z

java/vector/src/main/java/org/apache/arrow/vector/compare/VectorEqualsVisitor.java

I think there is an extra space here

BryanCutler · 2019-08-08T19:27:24Z

java/vector/src/main/java/org/apache/arrow/vector/compare/VectorEqualsVisitor.java

see previous comment - I think these could all be done in a single overridden compareValueVector

Thanks, fixed.

tianchen92 · 2019-08-09T09:03:21Z

Looks pretty good in general, thanks for doing this @tianchen92 ! I have a couple questions:

There seems to be some vectors that implement equals and the same code is also in RangeEqualsVisitor. Is this planned to be merged?

It's a little confusing to have both visitor and equals APIs for the same thing too. Should we keep the visitor API private?

It looks like we check names of child vectors for complex types only, and not the actual FieldVector name. Is that consistent with other languages like C++? I think I saw some discussion on this, but want to make sure

Thanks for your valuable comments, some clarifies as follows:

about equals API, we already has equals(index, vector, toIndex) to compare single values （now mainly used for dictionary encoding）. And in this PR I first though implement equals(ValueVector vector) to compare two vectors, and according to @pravindra suggestion，use RangeEqualsVisitor to replace equals(ValueVector vector). So no duplicated API here.
You are right, some code in RangeEqualsVisitor and in equals(index, vector, toIndex) are similar, I have extract a new class CompareUtility to make these logic reusable.
FieldVector names should be considered also, in VectorEqualsVisitor it compares Field which contains names and in RangeEqualsVisitor it compares FieldType without names. Do you think this is ok?

I updated this PR and please help take a look again :) @BryanCutler @pravindra Thanks a lot!

BryanCutler

Thanks @tianchen92 . I think for now, just stick to comparing vector values and leave the field name as a followup. Also, I don't think we should add another class CompareUtility since you could make use of the visitor in equals

BryanCutler · 2019-08-09T21:46:36Z

java/vector/src/main/java/org/apache/arrow/vector/compare/CompareUtility.java

I'm not crazy about adding another class to compare vectors

Right, removed.

BryanCutler · 2019-08-09T21:55:22Z

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

Instead of making CompareUtility to put common code, what about just making a RangeEqualsVisitor with a range of 1 and calling this from equals? or the other way around to call equals for an index from RangeEqualsVisitor? I slightly prefer the first, but I definitely don't think we should add more classes for this..

Thanks for suggestion, looks reasonable, fixed.

BryanCutler · 2019-08-09T21:58:39Z

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

I must have missed the discussion when equals was added to compare a single value. It doesn't seem like that would be very useful to me, was there a specific use case to support this?

BryanCutler · 2019-08-09T22:03:48Z

java/vector/src/main/java/org/apache/arrow/vector/compare/VectorEqualsVisitor.java

I'm not sure if the VectorEqualsVisitor should also compare Fields, specifically the name. But it shouldn't be done here, but in compareFieldVectors(FieldVector left...). Here, I think you should just call the super method. We can discuss comparing the names as a followup.

Sure, fixed.

…ectors equal

tianchen92 · 2019-08-10T02:30:36Z

Thanks @tianchen92 . I think for now, just stick to comparing vector values and leave the field name as a followup. Also, I don't think we should add another class CompareUtility since you could make use of the visitor in equals

Thanks for your quick reply, I updated this PR, please help take another look, thanks!

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

pravindra · 2019-08-12T07:20:50Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

+      int offsetWidth = BaseRepeatedValueVector.OFFSET_WIDTH;
+
+      if (!isNull) {
+        final int startByteLeft = left.getOffsetBuffer().getInt(leftIndex * offsetWidth);


nit (pls ignore if you don't agree): names are a bit confusing - startIndexLeft/endIndexLeft ?

Thanks, would also be resolved together in follow-ups.

pravindra

I've opened new jiras for the follow up items

https://issues.apache.org/jira/browse/ARROW-6210
https://issues.apache.org/jira/browse/ARROW-6211

tianchen92 · 2019-08-12T08:48:48Z

I've opened new jiras for the follow up items

https://issues.apache.org/jira/browse/ARROW-6210
https://issues.apache.org/jira/browse/ARROW-6211

Thanks a lot!

pravindra · 2019-08-12T08:51:34Z

thanks @tianchen92 for the change, and for meticulously following up on all the review comments !

BryanCutler

Thanks @tianchen92 this LGTM

Related to [ARROW-6210](https://issues.apache.org/jira/browse/ARROW-6210). This is a follow-up from #4933 The callers should be fixed to use the RangeEquals API instead. Closes #5065 from tianchen92/ARROW-6210 and squashes the following commits: cae440a <tianchen> resolve comments ad2277b <tianchen> resolve comments and port tests fb1510e <tianchen> add ValueMatcher 14b3e67 <tianchen> ARROW-6210: remove equals API from ValueVector Authored-by: tianchen <niki.lj@alibaba-inc.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

…Vector interface Related to [ARROW-6211](https://issues.apache.org/jira/browse/ARROW-6211). This is a follow-up from #4933 public interface VectorVisitor<OUT, IN, EX extends Exception> {..} n ValueVector : public <OUT, IN, EX extends Exception> OUT accept(VectorVisitor<OUT, IN, EX> visitor, IN value) throws EX; Closes #5091 from tianchen92/ARROW-6211 and squashes the following commits: 15a5231 <tianchen> fix bce258e <tianchen> add equals in RangeEqualsVisitor dbaa81f <tianchen> ARROW-6211: Remove dependency on RangeEqualsVisitor from ValueVector interface Authored-by: tianchen <niki.lj@alibaba-inc.com> Signed-off-by: Pindikura Ravindra <ravindra@dremio.com>

…Vector interface Related to [ARROW-6211](https://issues.apache.org/jira/browse/ARROW-6211). This is a follow-up from apache/arrow#4933 public interface VectorVisitor<OUT, IN, EX extends Exception> {..} n ValueVector : public <OUT, IN, EX extends Exception> OUT accept(VectorVisitor<OUT, IN, EX> visitor, IN value) throws EX; Closes #5091 from tianchen92/ARROW-6211 and squashes the following commits: 15a523177 <tianchen> fix bce258efc <tianchen> add equals in RangeEqualsVisitor dbaa81fc8 <tianchen> ARROW-6211: Remove dependency on RangeEqualsVisitor from ValueVector interface Authored-by: tianchen <niki.lj@alibaba-inc.com> Signed-off-by: Pindikura Ravindra <ravindra@dremio.com>

…ectors equal Related to [ARROW-6022](https://issues.apache.org/jira/browse/ARROW-6022). In some case, this feature is useful. In ARROW-1184, Dictionary#equals not work due to the lack of this API. Moreover, we already implemented equals(int index, ValueVector target, int targetIndex), so this new added API could reuse it. Closes apache#4933 from tianchen92/ARROW-6022 and squashes the following commits: 7e20f79 <tianchen> remove CompareUtility a5d22fd <tianchen> fix variable names 226a20f <tianchen> refactor 694d9f6 <tianchen> make equals to visitor mode 0dfa943 <tianchen> compare struct child names and add UT c7081c2 <tianchen> check list validity bit b942794 <tianchen> use ArrowType for equal 1d95c9c <tianchen> fix Decimal equals 3c9f066 <tianchen> fix 0026882 <tianchen> use MinorType and check isSet e58c158 <tianchen> refactor Dictionary#equals 10dca2c <tianchen> fix 6bc3f68 <tianchen> ARROW-6022: Support equals API in ValueVector to compare two vectors equal Authored-by: tianchen <niki.lj@alibaba-inc.com> Signed-off-by: Pindikura Ravindra <ravindra@dremio.com>

Related to [ARROW-6210](https://issues.apache.org/jira/browse/ARROW-6210). This is a follow-up from apache#4933 The callers should be fixed to use the RangeEquals API instead. Closes apache#5065 from tianchen92/ARROW-6210 and squashes the following commits: cae440a <tianchen> resolve comments ad2277b <tianchen> resolve comments and port tests fb1510e <tianchen> add ValueMatcher 14b3e67 <tianchen> ARROW-6210: remove equals API from ValueVector Authored-by: tianchen <niki.lj@alibaba-inc.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

tianchen92 mentioned this pull request Jul 25, 2019

ARROW-1184: [Java] Dictionary.equals is not working correctly #4843

Closed

tianchen92 force-pushed the ARROW-6022 branch 2 times, most recently from 383a56b to 6033ac8 Compare July 25, 2019 06:52

emkornfield reviewed Jul 26, 2019

View reviewed changes

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

tianchen92 force-pushed the ARROW-6022 branch from 6033ac8 to bc4abf2 Compare July 29, 2019 07:52

pravindra reviewed Jul 31, 2019

View reviewed changes

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

fsaintjacques added the Component: Java label Jul 31, 2019

tianchen92 force-pushed the ARROW-6022 branch from 7753e5b to 352b296 Compare August 1, 2019 07:36

pravindra reviewed Aug 5, 2019

View reviewed changes

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

pravindra reviewed Aug 5, 2019

View reviewed changes

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

pravindra reviewed Aug 5, 2019

View reviewed changes

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java Outdated Show resolved Hide resolved

pravindra reviewed Aug 8, 2019

View reviewed changes

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

praveenbingo reviewed Aug 8, 2019

View reviewed changes

BryanCutler reviewed Aug 8, 2019

View reviewed changes

tianchen92 force-pushed the ARROW-6022 branch from d1a79d7 to 6b6a269 Compare August 9, 2019 08:34

BryanCutler reviewed Aug 9, 2019

View reviewed changes

ARROW-6022: [Java] Support equals API in ValueVector to compare two v…

6bc3f68

…ectors equal

tianchen92 added 11 commits August 10, 2019 09:52

refactor Dictionary#equals

e58c158

use MinorType and check isSet

0026882

fix

3c9f066

fix Decimal equals

1d95c9c

use ArrowType for equal

b942794

check list validity bit

c7081c2

compare struct child names and add UT

0dfa943

make equals to visitor mode

694d9f6

refactor

226a20f

fix variable names

a5d22fd

remove CompareUtility

7e20f79

tianchen92 force-pushed the ARROW-6022 branch from a4e9c48 to 7e20f79 Compare August 10, 2019 02:21

pravindra reviewed Aug 12, 2019

View reviewed changes

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

pravindra reviewed Aug 12, 2019

View reviewed changes

pravindra approved these changes Aug 12, 2019

View reviewed changes

pravindra closed this in 6c1fccd Aug 12, 2019

tianchen92 mentioned this pull request Aug 12, 2019

ARROW-6210: [Java] remove equals API from ValueVector #5065

Closed

BryanCutler reviewed Aug 12, 2019

View reviewed changes

tianchen92 mentioned this pull request Aug 15, 2019

ARROW-6211: [Java] Remove dependency on RangeEqualsVisitor from ValueVector interface #5091

Closed

This was referenced Aug 12, 2019

[Java] Support equals API in ValueVector to compare two vectors equal #22428

Closed

[Java] remove equals API from ValueVector #22599

Closed

[Java] Remove dependency on RangeEqualsVisitor from ValueVector interface #22600

Closed

ARROW-6022: [Java] Support equals API in ValueVector to compare two vectors equal #4933

ARROW-6022: [Java] Support equals API in ValueVector to compare two vectors equal #4933

Uh oh!

Conversation

tianchen92 commented Jul 24, 2019

Uh oh!

Uh oh!

emkornfield commented Jul 26, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

tianchen92 commented Jul 31, 2019

Uh oh!

Uh oh!

pravindra commented Aug 1, 2019

Uh oh!

tianchen92 commented Aug 2, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pravindra commented Aug 5, 2019

Uh oh!

tianchen92 commented Aug 5, 2019

Uh oh!

pravindra commented Aug 8, 2019

Uh oh!

Uh oh!

tianchen92 commented Aug 8, 2019

Uh oh!

pravindra commented Aug 8, 2019

Uh oh!

tianchen92 commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler 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!

tianchen92 Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

codecov-io commented Jul 29, 2019 •

edited

Loading

tianchen92 Aug 10, 2019 •

edited

Loading