-
Couldn't load subscription status.
- Fork 3.9k
ARROW-6472: [Java] ValueVector#accept may has potential cast exception #5483
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.
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
Conversation
|
cc @pravindra @liyafan82 please help take a look, thanks! |
|
@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; |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with @liyafan82
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the clarification.
Sure. This is exactly what I was asking for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
thanks for fixing this, @tianchen92 |
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>
Related to ARROW-6472.
If we use visitor API this way:
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