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

tianchen92 · 2019-07-10T09:14:43Z

Related to ARROW-1184.
The Dictionary.equals method does not return True when the dictionaries are equal. This is because equals is not implemented for FieldVector and so that comparison defaults to comparing the two objects only and not the vector data.

BryanCutler

Thanks for doing this! Can you add a test with some encoded strings also?

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

tianchen92 · 2019-07-12T02:16:28Z

Thanks for doing this! Can you add a test with some encoded strings also?

Thanks, Encoded strings equals are not supported yet and I remarked it as TODO and will do it after #4792 merged, that PR introduces a ByteArrayBuffer for byte[] compare.

tianchen92 · 2019-07-12T02:17:21Z

@BryanCutler Thanks a lot for your feedback, I updated this PR, would you mind taking a look again? thanks!

tianchen92 · 2019-07-13T06:57:34Z

@emkornfield would you mind take a look at this one? thanks :)

BryanCutler

@tianchen92 thanks for doing this. I don't want to open a can of worms, but I think to do this right we need to address equals for vectors first and reconcile with what is already in vector.util.Validator. There should also be some unit tests for these across vector types.

If you want to fix up Dictionary.equals to call vector.util.Validator for now as a bandaid, I guess that will be ok

BryanCutler · 2019-07-15T19:20:26Z

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

This is mostly implemented in vector.util.Validator, which is used in Integration tests. It should probably be refactored, but it is ok to just call the functions for now, e.g. compareFieldVectors.

tianchen92 · 2019-07-16T06:39:48Z

@BryanCutler Thanks a lot for your comments.
Agreed that need implement equals for vector which I am doing in another one #4846.
This one as a bandaid, I refactor Validator logic (make it return boolean value instead of throwing Exception) and call it in Dictionary#equals. And since #4792 has been merged, make equals support for binary type and also add string and binary UT.
Would you mind taking a look again if we could get this merged? thanks a lot!

BryanCutler

It looks good in general, but I think you should also be doing the same thing for DictionaryEncoding equals. Also, the integration test failure looks relevant, can you check what went wrong?

BryanCutler · 2019-07-16T18:45:26Z

java/tools/src/test/java/org/apache/arrow/tools/TestIntegration.java

Can you explain this change? I don't the the exception message should be changing

Before this change, Validator could throw Exception with different message, since we make it return boolean value, log the explicit message instead of throwing it.

BryanCutler · 2019-07-16T18:50:54Z

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

It would be better to compare encodings with the Validator also, e.g. a function Validator.compareDictionaryEncodings. Isn't Object.equals incorrect here too?

I think Object.equals works fine with DictionaryEncoding since it implemented hashCode & equals already.

Looking at it further, DictionaryEncoder implements equals so I think you should call that.

Do you mean DictionaryEncoding? I think Object.equals will call DictionaryEncoding#equals?

My mistake, you are correct

tianchen92 · 2019-07-17T06:13:33Z

It looks good in general, but I think you should also be doing the same thing for DictionaryEncoding equals. Also, the integration test failure looks relevant, can you check what went wrong?

Thanks a lot for your comments, the relevant test failure is fixed now.

BryanCutler

@tianchen92 I didn't realize you would need to change the exceptions to logs to use the Validator. I don't think we should make that drastic of a change here because it is pretty important to integration tests. I think you should revert the changes to Validator/Integration and we have to options to work around:

As a temporary fix, just call Validator.compareFieldVectors and handle the exception in Dictionary.equals
Move the logic in Validator.compareFieldVectors to FieldVector.equals and call that from Validator and Dictionary.equals

You could also do (1) now and (2) as a followup. What do you think?

BryanCutler · 2019-07-19T02:49:45Z

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

Looking at it further, DictionaryEncoder implements equals so I think you should call that.

BryanCutler · 2019-07-19T02:57:31Z

java/vector/src/main/java/org/apache/arrow/vector/util/Validator.java

It's pretty important to be able to see the different error messages when an integration test fails and I'm not sure only logging them is good enough.

If we want to keep original error message, it might not proper to refactor Validator to make it has return type and use it in Dictionary, just copy it logic to Dictionary as previous commit does? Or any good suggestions? Thanks!

tianchen92 · 2019-07-19T03:14:28Z

@tianchen92 I didn't realize you would need to change the exceptions to logs to use the Validator. I don't think we should make that drastic of a change here because it is pretty important to integration tests. I think you should revert the changes to Validator/Integration and we have to options to work around:

As a temporary fix, just call Validator.compareFieldVectors and handle the exception in Dictionary.equals

Move the logic in Validator.compareFieldVectors to FieldVector.equals and call that from Validator and Dictionary.equals

You could also do (1) now and (2) as a followup. What do you think?

Sure, sounds reasonable, thanks!

tianchen92 · 2019-07-19T03:50:43Z

@BryanCutler Thanks for your effort, PR updated.

codecov-io · 2019-07-19T15:42:18Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4843      +/-   ##
==========================================
+ Coverage   87.44%   89.58%   +2.13%     
==========================================
  Files         995      661     -334     
  Lines      140479    96636   -43843     
  Branches     1418        0    -1418     
==========================================
- Hits       122841    86570   -36271     
+ Misses      17276    10066    -7210     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/arrow/json/converter.cc	`90.53% <0%> (-1.78%)`	⬇️
cpp/src/arrow/json/chunked-builder.cc	`79.91% <0%> (-1.68%)`	⬇️
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 327 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 e1559f2...83242dc. Read the comment docs.

codecov-io · 2019-07-19T15:42:18Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4843      +/-   ##
==========================================
+ Coverage   87.44%   89.58%   +2.13%     
==========================================
  Files         995      661     -334     
  Lines      140479    96636   -43843     
  Branches     1418        0    -1418     
==========================================
- Hits       122841    86570   -36271     
+ Misses      17276    10066    -7210     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/arrow/json/converter.cc	`90.53% <0%> (-1.78%)`	⬇️
cpp/src/arrow/json/chunked-builder.cc	`79.91% <0%> (-1.68%)`	⬇️
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 327 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 e1559f2...83242dc. Read the comment docs.

BryanCutler · 2019-07-22T17:55:24Z

@tianchen92 looks like there was a problem with your merge, can you rebase with master?

tianchen92 · 2019-07-23T08:06:53Z

@tianchen92 looks like there was a problem with your merge, can you rebase with master?

Sure, rebased.

BryanCutler

Looks good, but there is just one change I'm not sure of and seem to be a test failure

BryanCutler · 2019-07-23T17:23:30Z

java/vector/src/main/java/org/apache/arrow/vector/util/Validator.java

Why change this?

Field implemented equals method but it also compare names, that is to say, two same type vector with different name will not equal here even they have same elements, so compare with getClass is better?

We need to compare Field names for the integration tests. Ideally, we should be able to compare just the data in ValueVectors and the entire FieldVector also. For the dictionary vector, I don't think the field name is important, but shouldn't hurt to include the name check as well. Were you seeing a test failure if you revert this?

Like https://github.com/apache/arrow/pull/4843/files#diff-945722295a5f95e681897e48eedfcb08R459 vectors with different names ("v1" "v2") will not equals.
If you think we shouldn't change behavior of Validator, maybe we could refactor this PR and do not use Validator logic after #4933 merged, that PR introduces equals API in ValueVector. What do you think and could you also take a look? thanks!

I would rather not change the behavior of Validator here, can you revert this and we can make it less restrictive later?

Sure, fixed.

BryanCutler · 2019-07-23T17:34:38Z

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

Could you add a TODO note here to clean this up once we can compare vectors without using the Validator?

Thanks, fixed.

BryanCutler

LGTM

BryanCutler · 2019-07-25T06:08:55Z

CI tests all passed, merged to master. Thanks @tianchen92 !

Related to [ARROW-1184](https://issues.apache.org/jira/browse/ARROW-1184). The Dictionary.equals method does not return True when the dictionaries are equal. This is because equals is not implemented for FieldVector and so that comparison defaults to comparing the two objects only and not the vector data. Closes apache#4843 from tianchen92/ARROW-1184 and squashes the following commits: 3511857 <tianchen> revert 527a9d7 <tianchen> add TODO 0c69588 <tianchen> fix equals 1f41366 <tianchen> fix build 87b7f66 <tianchen> fix 3fe9389 <tianchen> use Validator logic in Dictionary#equals and add UT be74915 <tianchen> move UT 314d697 <tianchen> ARROW-1184: Dictionary.equals is not working correctly Authored-by: tianchen <niki.lj@alibaba-inc.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

BryanCutler requested changes Jul 11, 2019

View reviewed changes

BryanCutler reviewed Jul 11, 2019

View reviewed changes

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

tianchen92 force-pushed the ARROW-1184 branch from 1d10f25 to 66d99cc Compare July 12, 2019 08:11

fsaintjacques added the Component: Java label Jul 12, 2019

BryanCutler requested changes Jul 15, 2019

View reviewed changes

tianchen92 force-pushed the ARROW-1184 branch from 66d99cc to 5d26a39 Compare July 16, 2019 06:29

BryanCutler requested changes Jul 16, 2019

View reviewed changes

tianchen92 closed this Jul 17, 2019

tianchen92 reopened this Jul 17, 2019

BryanCutler requested changes Jul 19, 2019

View reviewed changes

tianchen92 force-pushed the ARROW-1184 branch from 670cc73 to 55dafce Compare July 19, 2019 03:43

tianchen92 force-pushed the ARROW-1184 branch from 89e5304 to 83242dc Compare July 19, 2019 06:23

kszucs force-pushed the master branch from 85fe336 to ed180da Compare July 22, 2019 17:04

kszucs force-pushed the master branch from ed180da to 85fe336 Compare July 22, 2019 19:29

tianchen92 force-pushed the ARROW-1184 branch from 83242dc to 1a4f3d0 Compare July 23, 2019 02:26

BryanCutler reviewed Jul 23, 2019

View reviewed changes

tianchen92 force-pushed the ARROW-1184 branch from efd9577 to b005bbf Compare July 24, 2019 03:50

tianchen92 added 2 commits July 25, 2019 11:27

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

314d697

move UT

be74915

tianchen92 added 6 commits July 25, 2019 11:27

use Validator logic in Dictionary#equals and add UT

3fe9389

fix

87b7f66

fix build

1f41366

fix equals

0c69588

add TODO

527a9d7

revert

3511857

tianchen92 force-pushed the ARROW-1184 branch from b005bbf to 3511857 Compare July 25, 2019 03:28

BryanCutler approved these changes Jul 25, 2019

View reviewed changes

BryanCutler closed this in f4fcf56 Jul 25, 2019

asfimport mentioned this pull request Aug 1, 2019

[Java] Dictionary.equals is not working correctly #16895

Closed

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

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

Uh oh!

Conversation

tianchen92 commented Jul 10, 2019

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tianchen92 commented Jul 12, 2019

Uh oh!

tianchen92 commented Jul 12, 2019

Uh oh!

tianchen92 commented Jul 13, 2019

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianchen92 commented Jul 16, 2019

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!

tianchen92 commented Jul 17, 2019

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!

tianchen92 commented Jul 19, 2019

Uh oh!

tianchen92 commented Jul 19, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

BryanCutler commented Jul 22, 2019

Uh oh!

tianchen92 commented Jul 23, 2019

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

codecov-io commented Jul 19, 2019 •

edited

Loading

codecov-io commented Jul 19, 2019 •

edited

Loading

tianchen92 Jul 24, 2019 •

edited

Loading

tianchen92 Jul 25, 2019 •

edited

Loading