-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(spanner): invalid numeric should throw an error #3926
fix(spanner): invalid numeric should throw an error #3926
Conversation
923c486
to
3852117
Compare
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'm not sure this is something that we can change now, at least not in this way. There might be users who are already using NUMERIC
and that rely (implicitly or explicitly) on the client library to automatically round any big.Rat
to a value that is acceptable for Spanner. Changing the behavior into returning an error in those cases is a breaking change, as it would suddenly cause those applications to receive an error from the client library where they previously got an OK.
One option could be to add this as a feature that users can use if they want to, for example by setting an option when creating the client, or maybe on a finer level (for example as an option on a statement?)
@olavloite I saw you changes (googleapis/java-spanner@9f26785) in Java which is also an improvement, right? How did you deal with the breaking change over there? |
That was before it was released, so we didn't have to worry about it. |
FYI: In the dotnet client the user is given the option whether to truncate or to throw an error if a In a later major version bump it might also be a possibility to change the default behavior. |
If I go with adding a flag // DisableLossOfPrecisionHandling is the configuration about how to handle
// values with significant digits that would be lost by converting from a
// number to a numeric. By default, loss of precision handling will be
// enabled, e.g., 1.1234567891 will be rounded to 1.123456789. If this is
// set to true, then an error will be raised.
DisableLossOfPrecisionHandling bool |
Hmmm.... Yup, that is going to be very difficult. As far as I can see now, the best option might be to add a global configuration option to Another option, albeit one that would require a lot more work, would be to add an additional type. It would be similar to My personal opinion is that the first option (adding a global configuration) would be sufficient for now, and that adding an extra type is something that we should only do if we know that there is a need for it. Regarding the option: I would recommend that it is defined as an |
@olavloite Thanks for the ideas. I had same thoughts with changing I didn't think about adding a new type I will try to add a global configuration option. |
3852117
to
f76b161
Compare
@olavloite Please take a look. I am still not sure if this is a good idea to add a global variable like this. It would be hard to remove in the future. I wonder if it is okay to not add this global variable, but let users to check the error by themselves, e.g., just make the I was considering to add a field to ClientConfig, which updates this global variable to avoid users to directly modify this variable. However, I realize that it would be confusing if a user creates multiple clients, with or without this field passing in. |
Yeah, this is not ideal, but I'm not sure that making the Another possibility might be to combine the two: So both having the global variable and make the
Yes, I agree that it is a good idea not to include it in the |
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 (with a small nit on the documentation for the rounding option)
@skuruppu Can I get an approval from you? Otherwise, I can't merge this. |
Fix #3920