Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,14 @@ public void setDoubleDiffFunction(DiffFunctionDouble doubleDiffFunction) {
@Override
public Boolean visit(BaseFixedWidthVector left, Range range) {
if (left instanceof Float4Vector) {
if (!validate(left)) {
return false;
}
return float4ApproxEquals(range);
} else if (left instanceof Float8Vector) {
if (!validate(left)) {
return false;
}
return float8ApproxEquals(range);
} else {
return super.visit(left, range);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,11 @@ public RangeEqualsVisitor(ValueVector left, ValueVector right, boolean isTypeChe
Preconditions.checkArgument(right != null,
"right vector cannot be null");

// types cannot change for a visitor instance. so, the check is done only once.
// type usually checks only once unless the left vector is changed.
checkType();
}

private void checkType() {
if (!isTypeCheckNeeded) {
typeCompareResult = true;
} else if (left == right) {
Expand All @@ -68,6 +72,17 @@ public RangeEqualsVisitor(ValueVector left, ValueVector right, boolean isTypeChe
}
}

/**
* Validate the passed left vector, if it is changed, reset and check type.
*/
protected boolean validate(ValueVector left) {
if (left != this.left) {
this.left = left;
checkType();
}
return typeCompareResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this way (a member boolean typeCompareResult) makes the code less readable. I am wondering why not make checkType method look like this:

boolean void checkType()

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with @liyafan82

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liyafan82 @pravindra Hmm, this member was just introduced by your previous PR. I think the reason is that cache this result in constructor and reused in rangeEqualsAPI each time.
See if we have good solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tianchen92 I see. Thanks for the clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the clarification.

}

/**
* Constructs a new instance.
*
Expand All @@ -79,7 +94,7 @@ public RangeEqualsVisitor(ValueVector left, ValueVector right) {
}

/**
* Check range equals without passing IN param in VectorVisitor.
* Check range equals.
*/
public boolean rangeEquals(Range range) {
if (!typeCompareResult) {
Expand Down Expand Up @@ -107,42 +122,59 @@ public ValueVector getRight() {
return right;
}

public boolean isTypeCheckNeeded() {
return isTypeCheckNeeded;
}

@Override
public Boolean visit(BaseFixedWidthVector left, Range range) {
if (!validate(left)) {
return false;
}
return compareBaseFixedWidthVectors(range);
}

@Override
public Boolean visit(BaseVariableWidthVector left, Range range) {
if (!validate(left)) {
return false;
}
return compareBaseVariableWidthVectors(range);
}

@Override
public Boolean visit(ListVector left, Range range) {
if (!validate(left)) {
return false;
}
return compareListVectors(range);
}

@Override
public Boolean visit(FixedSizeListVector left, Range range) {
if (!validate(left)) {
return false;
}
return compareFixedSizeListVectors(range);
}

@Override
public Boolean visit(NonNullableStructVector left, Range range) {
if (!validate(left)) {
return false;
}
return compareStructVectors(range);
}

@Override
public Boolean visit(UnionVector left, Range range) {
if (!validate(left)) {
return false;
}
return compareUnionVectors(range);
}

@Override
public Boolean visit(ZeroVector left, Range range) {
if (!validate(left)) {
return false;
}
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,31 @@ public void testIntVectorEqualsWithNull() {
}
}

@Test
public void testEqualsWithTypeChange() {
try (final IntVector vector1 = new IntVector("intVector1", allocator);
final IntVector vector2 = new IntVector("intVector2", allocator);
final BigIntVector vector3 = new BigIntVector("bigIntVector", allocator)) {

vector1.allocateNew(2);
vector1.setValueCount(2);
vector2.allocateNew(2);
vector2.setValueCount(2);

vector1.setSafe(0, 1);
vector1.setSafe(1, 2);

vector2.setSafe(0, 1);
vector2.setSafe(1, 2);

RangeEqualsVisitor visitor = new RangeEqualsVisitor(vector1, vector2);
Range range = new Range(0, 0, 2);
assertTrue(vector1.accept(visitor, range));
// visitor left vector changed, will reset and check type again
assertFalse(vector3.accept(visitor, range));
}
}

@Test
public void testBaseFixedWidthVectorRangeEqual() {
try (final IntVector vector1 = new IntVector("int", allocator);
Expand Down