Skip to content
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

Bugfix/fix array accessor in tuple #1248

Closed

Conversation

zero88
Copy link

@zero88 zero88 commented Oct 28, 2022

Motivation:

Fix vert-x3/vertx-jdbc-client#290
Fix #1247

@zero88 zero88 force-pushed the bugfix/fix-array-accessor-in-tuple branch from e051c86 to 966fd7d Compare October 28, 2022 06:09
@zero88
Copy link
Author

zero88 commented Oct 28, 2022

Hi @vietj , could you pls approve to run CI test

@zero88
Copy link
Author

zero88 commented Oct 28, 2022

Ah, actually some integration test fails related to ColumnChecker
Let me re-check, have not yet understood this assertion

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thank you @zero88 , a first round of comments

for (Object o : array) {
list.add(o);
}
List<Object> list = new ArrayList<>(array.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not bring anything, please revert it.

@tsegismont tsegismont self-assigned this Nov 11, 2022
@zero88 zero88 force-pushed the bugfix/fix-array-accessor-in-tuple branch from 966fd7d to e1d32eb Compare November 12, 2022 06:15
@zero88
Copy link
Author

zero88 commented Nov 12, 2022

After reviewing Tuple implementation, I think many inconsistent between getXX and getArrayOfXX, and it is very cumbersome.
For example, getXX has the coercion converter from another type to the target type but getArrayOfXX is missing.

Then, I propose ChainConverter to make the implementation simpler.

Could you have a quick look before I fix the integration test? @vietj @tsegismont

@zero88 zero88 force-pushed the bugfix/fix-array-accessor-in-tuple branch from e1d32eb to c3ea243 Compare November 12, 2022 09:34
} else {
return (Short) val; // Throw CCE
}
return ChainConverter.allowCast(Short.class)
Copy link
Member

Choose a reason for hiding this comment

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

this seems to not be GC free

Copy link
Author

Choose a reason for hiding this comment

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

Could you pls give more hint? I have not yet understood

Copy link
Author

Choose a reason for hiding this comment

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

ChainConverter is a short live object, that should deactivate and no reference after the method is terminated.
So, I thought it is not a big deal

@vietj
Copy link
Member

vietj commented Nov 14, 2022

we should avoid using primitive arrays when tuple are constructed instead of trying to deal with them.

@vietj vietj closed this Nov 14, 2022
@zero88
Copy link
Author

zero88 commented Nov 17, 2022

Hi @vietj
These are 2 different issues.

The first one is a primitive array

Actually, I can do

Tuple t = Tuple.of(new int[]{1, 2, 3})
t.getArrayOfIntegers(0)

But actually, no "reality" use case for this on SQL. The prepared parameters can be array, but most SQL client (e.g JDBC) uses an Object array instead of a primitive array.

So, I agree about

we should avoid using primitive arrays with Tuple

@zero88
Copy link
Author

zero88 commented Nov 17, 2022

The second one is vert-x3/vertx-jdbc-client#290
Should Tuple store a real object and delegate to each method can convert it to the expected type?

For example:
In the query for the integer array field, the actual value is Integer{}[1, 2, 3], then

  • getObject can return Integer{}[1, 2, 3]
  • getJsonArray can return JsonArray.of(1, 2, 3)
  • getArrayOfIntegers can return Integer{}[1, 2, 3], and the same for coercion methods such as getArrayOfShorts, etc..

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.

getJsonArray() throws ClassCastException from v4.2.0 upwards Tuple is not working if access primitive array
4 participants