Skip to content

Conversation

@tianchen92
Copy link
Contributor

Related to 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:
#5195 (comment)
https://issues.apache.org/jira/browse/ARROW-6472

@tianchen92
Copy link
Contributor Author

cc @pravindra @liyafan82 please help take a look, thanks!

@liyafan82
Copy link
Contributor

@tianchen92 can you please provide a test that fails without your patch?

@tianchen92
Copy link
Contributor Author

@tianchen92 can you please provide a test that fails without your patch?

Thanks for reminder, since the potential exception is obvious as I commented above and hard to construct failure test with this patch in. I prefer to add UT(left vector changed) which is expected to be successful to validate this patch.

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.

@liyafan82
Copy link
Contributor

@tianchen92 can you please provide a test that fails without your patch?

Thanks for reminder, since the potential exception is obvious as I commented above and hard to construct failure test with this patch in. I prefer to add UT(left vector changed) which is expected to be successful to validate this patch.

Sure. This is exactly what I was asking for.

Copy link
Contributor

@pravindra pravindra left a comment

Choose a reason for hiding this comment

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

lgtm

@pravindra pravindra closed this in 6dec194 Sep 25, 2019
@pravindra
Copy link
Contributor

thanks for fixing this, @tianchen92

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
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 <niki.lj@alibaba-inc.com>
Signed-off-by: Pindikura Ravindra <ravindra@dremio.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants