[Java] ValueVector#accept may has potential cast exception

[asfimport]

Per discussion #5195 (comment)

We may use API this way:

RangeEqualsVisitor visitor = new RangeEqualsVisitor(vector1, vector2);
vector3.accept(visitor, range)

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

Reporter: Ji Liu / @tianchen92
Assignee: Ji Liu / @tianchen92

PRs and other links:

• GitHub Pull Request #5483

_{Note: This issue was originally created as ARROW-6472. Please see the migration documentation for further details.}

[asfimport]

Ji Liu / @tianchen92:
cc @pravindra [~fan_li_ya] for comments on this issue, do you have any points how to resolve this problem in your mind?

Here are some my initial thoughts:

I am against that visitors keep both leftVector and rightVector for some reasons:

i. make visitor and accept API inconsistent as we mentioned before

ii. I think visitor should be reused for multiple leftVectors (with one visitor, vector1.accept(visitor, IN), vector2.accept(visitor, IN) …), and this way, the left and right vectors are fixed.

 

If we remove leftVector and pass it in via accept API, the main problem we should solve is repeated type checks in ListVector, we could do something to avoid this:

i. Keep Range param since it makes it possible for compare different ranges without creating extra visitor instance.

ii. Move typeCheck flag into Range(default is true), in cases compare ListVector data vector, change the flag after first compare and we could skip type checks in follow-up compares according to this flag.

 

This is only an initial thoughts and would be better if you have other points.

If we could not find a proper solution or reach a consistent in this JIRA, it‘s ok for me to start a discuss in ML.

Thanks!

[asfimport]

Liya Fan / @liyafan82:
My suggestions:

1. We need to use both left and right vectors in the constructors. The reason is for performance: this is a short-cut and avoids repeated type check.

2. For each visit invocation, we first check if the parameter is the same as the left vector (which is usually true). If it is true, we go ahead to call the compare methods. If it is false, we 1) override the left vector; 2) do the type check; 3) call the compare methods.

public void visit(Vector left, Range range) {
if (left != this.left) {
this.left = left;
typeCheck();
}
compareXXX();
}
This method keeps the good performance, while avoiding the problem indicated by [~Pindikura Ravindra].

What do you think?

[asfimport]

Ji Liu / @tianchen92:
cc @pravindra  What do you think Liya's suggestion? If you have no objections, I would fix that way.

[asfimport]

Pindikura Ravindra / @pravindra:
Issue resolved by pull request 5483
#5483