Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tianchen92 wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6bc3f68
ARROW-6022: [Java] Support equals API in ValueVector to compare two v…
Jul 24, 2019
10dca2c
fix
Jul 24, 2019
e58c158
refactor Dictionary#equals
Jul 25, 2019
0026882
use MinorType and check isSet
Jul 29, 2019
3c9f066
fix
Jul 29, 2019
1d95c9c
fix Decimal equals
Jul 29, 2019
b942794
use ArrowType for equal
Jul 31, 2019
c7081c2
check list validity bit
Aug 5, 2019
0dfa943
compare struct child names and add UT
Aug 5, 2019
694d9f6
make equals to visitor mode
Aug 8, 2019
226a20f
refactor
Aug 9, 2019
a5d22fd
fix variable names
Aug 9, 2019
7e20f79
remove CompareUtility
Aug 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions java/vector/src/main/codegen/templates/UnionVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
import io.netty.buffer.ArrowBuf;
import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.ReferenceManager;
import org.apache.arrow.util.Preconditions;
import org.apache.arrow.vector.ValueVector;
import org.apache.arrow.vector.types.UnionMode;
import org.apache.arrow.vector.compare.RangeEqualsVisitor;
import org.apache.arrow.vector.types.pojo.FieldType;
import org.apache.arrow.vector.util.CallBack;

Expand All @@ -36,6 +38,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import org.apache.arrow.vector.compare.RangeEqualsVisitor;
import org.apache.arrow.vector.complex.impl.ComplexCopier;
import org.apache.arrow.vector.util.CallBack;
import org.apache.arrow.vector.ipc.message.ArrowFieldNode;
Expand Down Expand Up @@ -665,16 +668,17 @@ public boolean equals(int index, ValueVector to, int toIndex) {
if (to == null) {
return false;
}
if (this.getClass() != to.getClass()) {
return false;
}
UnionVector that = (UnionVector) to;
ValueVector leftVector = getVector(index);
ValueVector rightVector = that.getVector(toIndex);
Preconditions.checkArgument(index >= 0 && index < valueCount,
"index %s out of range[0, %s]:", index, valueCount - 1);
Preconditions.checkArgument(toIndex >= 0 && toIndex < to.getValueCount(),
"index %s out of range[0, %s]:", index, to.getValueCount() - 1);

if (leftVector.getClass() != rightVector.getClass()) {
return false;
}
return leftVector.equals(index, rightVector, toIndex);
RangeEqualsVisitor visitor = new RangeEqualsVisitor(to, index, toIndex, 1);
return this.accept(visitor);
}

@Override
public boolean accept(RangeEqualsVisitor visitor) {
return visitor.visit(this);
}
}
27 changes: 15 additions & 12 deletions java/vector/src/main/java/org/apache/arrow/vector/BaseFixedWidthVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.apache.arrow.memory.util.ArrowBufPointer;
import org.apache.arrow.memory.util.ByteFunctionHelpers;
import org.apache.arrow.util.Preconditions;
import org.apache.arrow.vector.compare.RangeEqualsVisitor;
import org.apache.arrow.vector.ipc.message.ArrowFieldNode;
import org.apache.arrow.vector.types.pojo.Field;
import org.apache.arrow.vector.util.CallBack;
Expand Down Expand Up @@ -71,6 +72,10 @@ public BaseFixedWidthVector(Field field, final BufferAllocator allocator, final
}


public int getTypeWidth() {
return typeWidth;
}

@Override
public String getName() {
return field.getName();
Expand Down Expand Up @@ -870,20 +875,18 @@ public boolean equals(int index, ValueVector to, int toIndex) {
if (to == null) {
return false;
}
if (this.getClass() != to.getClass()) {
return false;
}

BaseFixedWidthVector that = (BaseFixedWidthVector) to;
Preconditions.checkArgument(index >= 0 && index < valueCount,
"index %s out of range[0, %s]:", index, valueCount - 1);
Preconditions.checkArgument(toIndex >= 0 && toIndex < to.getValueCount(),
"index %s out of range[0, %s]:", index, to.getValueCount() - 1);

int leftStart = typeWidth * index;
int leftEnd = typeWidth * (index + 1);

int rightStart = typeWidth * toIndex;
int rightEnd = typeWidth * (toIndex + 1);
RangeEqualsVisitor visitor = new RangeEqualsVisitor(to, index, toIndex, 1);
return this.accept(visitor);
}

int ret = ByteFunctionHelpers.equal(this.getDataBuffer(), leftStart, leftEnd,
that.getDataBuffer(), rightStart, rightEnd);
return ret == 1;
@Override
public boolean accept(RangeEqualsVisitor visitor) {

@BryanCutler BryanCutler Aug 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tianchen92 tianchen92 Aug 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@BryanCutler BryanCutler Aug 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@tianchen92 tianchen92 Aug 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pravindra pravindra Aug 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tianchen92 tianchen92 Aug 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pravindra pravindra Aug 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tianchen92 tianchen92 Aug 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return visitor.visit(this);
}
}
24 changes: 11 additions & 13 deletions java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.arrow.memory.util.ArrowBufPointer;
import org.apache.arrow.memory.util.ByteFunctionHelpers;
import org.apache.arrow.util.Preconditions;
import org.apache.arrow.vector.compare.RangeEqualsVisitor;
import org.apache.arrow.vector.ipc.message.ArrowFieldNode;
import org.apache.arrow.vector.types.pojo.Field;
import org.apache.arrow.vector.util.CallBack;
Expand Down Expand Up @@ -1369,20 +1370,17 @@ public boolean equals(int index, ValueVector to, int toIndex) {
if (to == null) {
return false;
}
if (this.getClass() != to.getClass()) {
return false;
}

BaseVariableWidthVector that = (BaseVariableWidthVector) to;
Preconditions.checkArgument(index >= 0 && index < valueCount,
"index %s out of range[0, %s]:", index, valueCount - 1);
Preconditions.checkArgument(toIndex >= 0 && toIndex < to.getValueCount(),
"index %s out of range[0, %s]:", index, to.getValueCount() - 1);

final int leftStart = getStartOffset(index);
final int leftEnd = getStartOffset(index + 1);

final int rightStart = that.getStartOffset(toIndex);
final int rightEnd = that.getStartOffset(toIndex + 1);
RangeEqualsVisitor visitor = new RangeEqualsVisitor(to, index, toIndex, 1);
return this.accept(visitor);
}

int ret = ByteFunctionHelpers.equal(this.getDataBuffer(), leftStart, leftEnd,
that.getDataBuffer(), rightStart, rightEnd);
return ret == 1;
@Override
public boolean accept(RangeEqualsVisitor visitor) {
return visitor.visit(this);
}
}
1 change: 0 additions & 1 deletion java/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,6 @@ public void setSafe(int index, int isSet, int start, ArrowBuf buffer) {
set(index, isSet, start, buffer);
}


/*----------------------------------------------------------------*
| |
| vector transfer |
Expand Down
6 changes: 6 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/ExtensionTypeVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.OutOfMemoryException;
import org.apache.arrow.vector.compare.RangeEqualsVisitor;
import org.apache.arrow.vector.complex.reader.FieldReader;
import org.apache.arrow.vector.ipc.message.ArrowFieldNode;
import org.apache.arrow.vector.types.Types.MinorType;
Expand Down Expand Up @@ -256,4 +257,9 @@ public Iterator<ValueVector> iterator() {
public BufferAllocator getAllocator() {
return underlyingVector.getAllocator();
}

@Override
public boolean accept(RangeEqualsVisitor visitor) {
return visitor.visit(getUnderlyingVector());
}
}
8 changes: 8 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/ValueVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.OutOfMemoryException;
import org.apache.arrow.vector.compare.RangeEqualsVisitor;
import org.apache.arrow.vector.complex.reader.FieldReader;
import org.apache.arrow.vector.types.Types.MinorType;
import org.apache.arrow.vector.types.pojo.Field;
Expand Down Expand Up @@ -272,4 +273,11 @@ public interface ValueVector extends Closeable, Iterable<ValueVector> {
* @param from source vector
*/
void copyFromSafe(int fromIndex, int thisIndex, ValueVector from);

/**
* Compare range values in this vector and vector in visitor.
* @param visitor visitor which holds the vector to compare.
* @return true if equals, otherwise false.
*/
boolean accept(RangeEqualsVisitor visitor);
Comment thread
pravindra marked this conversation as resolved.
}
6 changes: 6 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/ZeroVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.OutOfMemoryException;
import org.apache.arrow.vector.compare.RangeEqualsVisitor;
import org.apache.arrow.vector.complex.impl.NullReader;
import org.apache.arrow.vector.complex.reader.FieldReader;
import org.apache.arrow.vector.ipc.message.ArrowFieldNode;
Expand Down Expand Up @@ -264,4 +265,9 @@ public void copyFrom(int fromIndex, int thisIndex, ValueVector from) {
public void copyFromSafe(int fromIndex, int thisIndex, ValueVector from) {
throw new UnsupportedOperationException();
}

@Override
public boolean accept(RangeEqualsVisitor visitor) {

@BryanCutler BryanCutler Aug 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tianchen92 tianchen92 Aug 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

return true;
}
}
Loading