ARROW-6738: [Java] Fix problems with current union comparison logic #5544

liyafan82 · 2019-09-30T11:00:27Z

There are some problems with the current union comparison logic. For example:

For type check, we should not require fields to be equal. It is possible that two vectors' value ranges are equal but their fields are different.
We should not compare the number of sub vectors, as it is possible that two union vectors have different numbers of sub vectors, but have equal values in the range.

github-actions · 2019-09-30T11:01:45Z

https://issues.apache.org/jira/browse/ARROW-6738

codecov-io · 2019-09-30T12:40:36Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #5544      +/-   ##
==========================================
+ Coverage   88.96%   89.97%      +1%     
==========================================
  Files         991      739     -252     
  Lines      135132   115054   -20078     
  Branches     1501        0    -1501     
==========================================
- Hits       120225   103520   -16705     
+ Misses      14542    11534    -3008     
+ Partials      365        0     -365

Impacted Files	Coverage Δ
python/pyarrow/plasma.py	`54.79% <0%> (-1.37%)`	⬇️
cpp/src/arrow/util/thread_pool_test.cc	`97.66% <0%> (-0.94%)`	⬇️
r/src/recordbatch.cpp
go/arrow/math/uint64_amd64.go
r/R/filesystem.R
r/R/list.R
r/src/symbols.cpp
r/src/array_to_vector.cpp
go/arrow/ipc/file_reader.go
js/src/builder/index.ts
... and 246 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 558263f...f27cec6. Read the comment docs.

pitrou · 2019-10-16T17:27:01Z

@jacques-n @emkornfield

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

java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java

emkornfield · 2019-10-17T05:23:58Z

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

I'm not sure it is correct to flip since the scale is different between the two vectors. @pravindra ?

This is a general question.
A numeric can be represented as different decimal values, with different scales and precisions.

The question is: is the same numeric value equal when represented in different format?

For BigDecimal in standard java, there are two ways to compare:

The equals method compares both value and scale.

The compareTo method comapres the underlying value.

From a performance view point, comparing by the underlying value is more performant for Arrow's memory layout?

My impression is the comparison is actually .1 == 1 and .2 == 2 (i.e not the same as java BigDecimal, I might be misreading it but if you look at BigDecimal.getObject, I think this agrees with my assessment).

@emkornfield You are right.
I am wrong because Arrow is different from BigDecimal. Arrow stores unscaled values.
Sorry for my mistake.

I have updated the code to revert this flip. Please take a look.

@emkornfield After more thoughts, I think UnionVector is a special case: for union vectors, it is possible that two vectors have different fields, but have equal values in a certain range.

So the changes for the RangeEqualsVisitor are like this: for union vectors, we only compare minor types, for other vectors, we compare fields.

The problem for DecimalVector no longer exisits.

emkornfield · 2019-10-17T05:25:40Z

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

shouldn't this only return on inequality?

nice catch. Thank you so much.

emkornfield · 2019-10-17T05:26:18Z

this needs a rebase and I'm not sure all the changes are correct.

liyafan82 · 2019-10-17T10:17:32Z

this needs a rebase and I'm not sure all the changes are correct.

Rebase done. Thank you.

emkornfield · 2019-10-24T06:07:56Z

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

by this argument, wouldn't we need to always return true here and validate types in equalities.
i.e. if left is a union and right is an intvector it union[1] happens to be an int vector the comparison should happen.

I think the argument only applies when both vectors are union vectors.
An IntVector and a UnionVector can not have equal range, even the values in the range are actually equal.

In C++, it also requires that both vectors are union vectors:

bool CompareUnions(const UnionArray& left) const {
const auto& right = checked_cast<const UnionArray&>(right_);

its been a while since I looked at the C++ code, but are you sure there isn't a check for union equality first in C++? It doesn't seem unreasonable to require the types be identical.

There is a check for union type euqlity (and other types of arrays) in C++, and it requres the type to be identical.

To see this, note in the above code snippet that the left vector must be a UnionArray (passed in as the parameter), and the right vector must also be a UnionArray, and this is ensured by

checked_cast<const UnionArray&>(right_);

I think maybe I was not answering your question, and there is something confusing here. So let me try to make it clearer:

There are two types of type equality:
shallow equality - which only requires the minor types to be the same
deep equality - which requires the minor type, minor types of child types, and order of child types be equal.

For Java, we check deep equality for other types of vectors, and for union vector, we only check shallow equality.

This is because, for union vector, it is possible that two union vectors may have different child vectors, but their values are equal in a certain range. This is not possible for other types of vectors.

Thanks for your comments. I think some points are a little clearer for me.

I think there are two (somewhat) independent problems here:

Should child vector types being equal be a necessary condition for vector ranges being equal.

Should we consider the above 3 (or other alternative) semantics proposed by you.

For me, 1 is unacceptable, as it causes practical problems (e.g. encoding a UnionVector). For 2, I do not prefer it, but it is acceptable for me.

I'm not sure they are independent. But at this point, if you still feel strongly about 1, please start a discussion on the mailing list.

Sure. Thanks for your suggestion. I will start a discussion in the ML.

It doesn't seem like there was much discussion on this in the mailing list, would you be willing to separate the fixes here, so we can merge the bug fix and maybe try bumping the thread on the mailing list?

Sounds like a good idea. I have revised the PR to undo the union type comparison logic.
It seems like a seprate issue, and may involve multiple languages.

Maybe I will do more investigations and start a new discussion to describe the problem in a clearer way.

This discussion have been for a long time. Thanks a lot for your patience.

liyafan82 · 2020-01-21T09:26:00Z

Rebased to resolve conflicts.

emkornfield · 2020-01-24T05:57:10Z

java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java

+      uInt4Holder.isSet = 1;
+
+      final NullableIntHolder intHolder = new NullableIntHolder();
+      uInt4Holder.value = 20;


was this intended to set intHolder?

You are right. Sorry about my mistake. Resolved now.

emkornfield · 2020-01-24T05:58:24Z

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


    for (String name : leftChildNames) {
-      RangeEqualsVisitor visitor = createInnerVisitor(leftVector.getChild(name), rightVector.getChild(name));
+      RangeEqualsVisitor visitor = createInnerVisitor(leftVector.getChild(name), rightVector.getChild(name), null);


please add a comment for what null represents (/param_name=/) here and below.

Comments added. Thanks for the good suggestion.

emkornfield · 2020-01-24T05:59:38Z

java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java

+      vector1.setValueCount(3);
+
+      vector2.setType(0, Types.MinorType.UINT4);
+      vector2.setSafe(0, uInt4Holder);


sorry if I'm missing it but these vectors do look identifical?

You are right. I have revised the test case so the two vectors are different.

There are some problems with the current union comparison logic. For example: 1. For type check, we should not require fields to be equal. It is possible that two vectors' value ranges are equal but their fields are different. 2. We should not compare the number of sub vectors, as it is possible that two union vectors have different numbers of sub vectors, but have equal values in the range. Closes #5544 from liyafan82/fly_0930_share and squashes the following commits: d6ef3d2 <liyafan82> Refine test case c008289 <liyafan82> Resolve test failure after rebasing c515393 <liyafan82> Rule out the change for union type comparison bab7402 <liyafan82> Compare fields for all vectors except union vectors 5b2225e <liyafan82> Fix the bug with decimal vector 4d8b570 <liyafan82> Fix problems with current union comparison logic Authored-by: liyafan82 <fan_li_ya@foxmail.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

There are some problems with the current union comparison logic. For example: 1. For type check, we should not require fields to be equal. It is possible that two vectors' value ranges are equal but their fields are different. 2. We should not compare the number of sub vectors, as it is possible that two union vectors have different numbers of sub vectors, but have equal values in the range. Closes apache#5544 from liyafan82/fly_0930_share and squashes the following commits: d6ef3d2 <liyafan82> Refine test case c008289 <liyafan82> Resolve test failure after rebasing c515393 <liyafan82> Rule out the change for union type comparison bab7402 <liyafan82> Compare fields for all vectors except union vectors 5b2225e <liyafan82> Fix the bug with decimal vector 4d8b570 <liyafan82> Fix problems with current union comparison logic Authored-by: liyafan82 <fan_li_ya@foxmail.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

fsaintjacques added the Component: Java label Oct 2, 2019

kszucs force-pushed the fly_0930_share branch from 3b0291e to f3c07d1 Compare October 5, 2019 10:07

emkornfield reviewed Oct 17, 2019

View reviewed changes

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

emkornfield reviewed Oct 17, 2019

View reviewed changes

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

emkornfield reviewed Oct 17, 2019

View reviewed changes

liyafan82 force-pushed the fly_0930_share branch from f3c07d1 to 3ebbf4c Compare October 17, 2019 10:05

emkornfield reviewed Oct 24, 2019

View reviewed changes

liyafan82 added 5 commits January 21, 2020 17:04

[ARROW-6738][Java] Fix problems with current union comparison logic

4d8b570

[ARROW-6738][Java] Fix the bug with decimal vector

5b2225e

[ARROW-6738][Java] Compare fields for all vectors except union vectors

bab7402

[ARROW-6738][Java] Rule out the change for union type comparison

c515393

[ARROW-6738][Java] Resolve test failure after rebasing

c008289

liyafan82 force-pushed the fly_0930_share branch from 1a6756d to c008289 Compare January 21, 2020 09:24

emkornfield reviewed Jan 24, 2020

View reviewed changes

[ARROW-6738][Java] Refine test case

d6ef3d2

emkornfield closed this in 942a4d0 Feb 3, 2020

asfimport mentioned this pull request Feb 3, 2020

[Java] Fix problems with current union comparison logic #23079

Closed

ARROW-6738: [Java] Fix problems with current union comparison logic #5544

ARROW-6738: [Java] Fix problems with current union comparison logic #5544

Uh oh!

Conversation

liyafan82 commented Sep 30, 2019

Uh oh!

github-actions bot commented Sep 30, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

pitrou commented Oct 16, 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!

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 Oct 17, 2019

Uh oh!

liyafan82 commented Oct 17, 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!

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!

liyafan82 commented Jan 21, 2020

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Sep 30, 2019 •

edited

Loading