Skip to content

Commit

Permalink
ARROW-6472: [Java] ValueVector#accept may has potential cast exception
Browse files Browse the repository at this point in the history
Related to [ARROW-6472](https://issues.apache.org/jira/browse/ARROW-6472).

If we use visitor API this way:
>RangeEqualsVisitor visitor = new RangeEqualsVisitor(vector1, vector2);
vector3.accept(visitor, range)

if vector1/vector2 are say, StructVector}}s and vector3 is an {{IntVector - things can go bad. we'll use the compareBaseFixedWidthVectors() and do wrong type-casts for vector1/vector2.

Discussions see:
apache#5195 (comment)
https://issues.apache.org/jira/browse/ARROW-6472

Closes apache#5483 from tianchen92/ARROW-6472 and squashes the following commits:

3d3d295 <tianchen> add test
12e4aa2 <tianchen> ARROW-6472:  ValueVector#accept may has potential cast exception

Authored-by: tianchen <[email protected]>
Signed-off-by: Pindikura Ravindra <[email protected]>
  • Loading branch information
tianchen92 authored and alippai committed Sep 26, 2019
1 parent 8e1d120 commit 1d573d9
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 6 deletions.
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;
}

/**
* 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

0 comments on commit 1d573d9

Please sign in to comment.