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

Let Result.getRowsUpdated() return Long #255

Closed
elefeint opened this issue Dec 3, 2021 · 3 comments
Closed

Let Result.getRowsUpdated() return Long #255

elefeint opened this issue Dec 3, 2021 · 3 comments
Labels
target: 1.0 Issues we want to address with 1.0.x. type: enhancement A general enhancement
Milestone

Comments

@elefeint
Copy link
Contributor

elefeint commented Dec 3, 2021

Currently, Result.getRowsUpdated() returns a Publisher<Integer>.

Cloud Spanner naturally returns Long (internally known as INT64) values, and the Cloud Spanner R2DBC driver works around the mismatch by converting too-large numbers with a warning and loss of original value.

Ideally, we would be able to pass the original value through to the end user. This would require the result specification to change from Publisher<Integer> getRowsUpdated() to Publisher<Long> getRowsUpdated().

This may also help future-proof the spec as datasets grow ever-larger.

@gregturn
Copy link
Contributor

gregturn commented Dec 3, 2021

This is also an issue for H2 because they now return a Long. I am force fitting it into an Integer to comply.

@Michael-A-McMahon
Copy link
Contributor

It's also an issue for Oracle R2DBC. We throw an ArithmeticException if an update count exceeds Integer.MAX_VALUE.

Publishing Long, rather than Integer, seems like the right thing to do. This way, it will be consistent with Result.UpdateCount.value():

The only reason I can think of not to do this would be a significant performance benefit from using Integer instead of Long. If that can be demonstrated, then there might be case for SPI methods that expose int-valued updated counts.

@mp911de
Copy link
Member

mp911de commented Dec 4, 2021

I'm wondering what's the best way to go about it without creating too much trouble. The SPI could specify Publisher<? extends Number> for the time being, even for 0.9 without breaking existing implementations and for 1.0 we could migrate to Long. That could give implementations some time to adopt.

Switching with 1.0 directly from Integer to Number would render implementations incompatible. What other possibilities do we have?

Likely, regardless of what we do, either drivers or clients will break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target: 1.0 Issues we want to address with 1.0.x. type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants