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

fix(spanner): invalid numeric should throw an error #3926

Merged
merged 8 commits into from Aug 23, 2021

Conversation

hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Apr 14, 2021

Fix #3920

@hengfengli hengfengli added the api: spanner Issues related to the Cloud Spanner API. label Apr 14, 2021
@hengfengli hengfengli requested a review from olavloite Apr 14, 2021
@hengfengli hengfengli self-assigned this Apr 14, 2021
@hengfengli hengfengli requested review from skuruppu and a team as code owners Apr 14, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 14, 2021
@hengfengli hengfengli force-pushed the add-invalid-numeric-error branch 3 times, most recently from 923c486 to 3852117 Compare Apr 14, 2021
Copy link
Contributor

@olavloite olavloite left a comment

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?)

@hengfengli
Copy link
Contributor Author

hengfengli commented Apr 15, 2021

@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?

@olavloite
Copy link
Contributor

olavloite commented Apr 15, 2021

@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.

@olavloite
Copy link
Contributor

olavloite commented Apr 15, 2021

FYI: In the dotnet client the user is given the option whether to truncate or to throw an error if a decimal is used that does not fit in a SpannerNumeric. I think that in this case that something similar would be the most logical option (although in this case it would probably be return error or round). To make it non-breaking, the default behavior would be to round a value that does not fit.

In a later major version bump it might also be a possibility to change the default behavior.

@hengfengli
Copy link
Contributor Author

hengfengli commented Apr 16, 2021

If I go with adding a flag DisableLossOfPrecisionHandling into the ClientConfig, is there any way to pass it into encodeValue without changing the function definition? I'd like to make minimum changes. I don't see an easy way to do that.

	// 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

@olavloite
Copy link
Contributor

olavloite commented Apr 16, 2021

If I go with adding a flag DisableLossOfPrecisionHandling into the ClientConfig, is there any way to pass it into encodeValue without changing the function definition? I'd like to make minimum changes. I don't see an easy way to do that.

	// 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. encodeValue is just a package-level exported function. There is no Encoder struct or anything like that where you could set the config from a specific client. And even if you were to change the function definition it would still be very difficult to pass in the correct parameter if we were to add this option to the ClientConfig. encodeValue is called from multiple places that are not related to a specific client. See for example Statement. A Statement is not related to a specific client, but does call encodeValue.

As far as I can see now, the best option might be to add a global configuration option to value.go that is not client-dependent. That would mean that an application can only change the behavior for all clients that it creates, and not on a per-client basis.

Another option, albeit one that would require a lot more work, would be to add an additional type. It would be similar to NullNumeric. So you could add something like SpannerNumeric that contains a big.Rat and an option that determines what to do if precision would be lost. This would mean adding support for SpannerNumeric everywhere where currently NullNumeric is used, which is quite a lot of places.

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 enum instead of a boolean. That way, it will be easier to add additional options for what to do when losing precision later without it having to be a breaking change. The current behavior is Round (which rounding mode?). The one that you want to add at the moment is Fail, but there might be a need in the future to add the option of other rounding modes and/or Truncate.

@hengfengli
Copy link
Contributor Author

hengfengli commented Apr 20, 2021

@olavloite Thanks for the ideas. I had same thoughts with changing encode_value function and adding a global option, both of which I am not very satisfied with.

I didn't think about adding a new type SpannerNumeric but I agree that this is not necessary at the moment.

I will try to add a global configuration option.

@hengfengli hengfengli force-pushed the add-invalid-numeric-error branch from 3852117 to f76b161 Compare May 1, 2021
@hengfengli
Copy link
Contributor Author

hengfengli commented May 1, 2021

@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 validateNumeric function public for users.

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.

@olavloite
Copy link
Contributor

olavloite commented May 1, 2021

@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 validateNumeric function public for users.

Yeah, this is not ideal, but I'm not sure that making the validateNumeric exported is a better solution than this global variable. Making validateNumeric exported would mean that we don't break anything for any users, and those who want to validate numeric input before sending it to Spanner can do so by calling this method. The disadvantage is that they would have to do it for all input, and missing one will cause it to be silently rounded.
Having the global variable has the disadvantage that it is not something that we can easily remove later, and a global variable for something like this also feels a little bit awkward. It would have been better if it would have been possible to configure it on the client, but I cannot see how that would be doable without a major rewrite of the entire type encoding and decoding. The advantage with a global variable is that users do not need to change anything to keep the current working, and they can easily switch it on in a test environment to check whether it is actually a problem for them or not at the moment.

Another possibility might be to combine the two: So both having the global variable and make the validateNumeric exported. That would allow users to validate their input at the source, which would make it easier for them to turn on the option at a later moment if they do not want any silent rounding.

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.

Yes, I agree that it is a good idea not to include it in the ClientConfig. That would be confusing if you create multiple clients.

spanner/value.go Outdated Show resolved Hide resolved
spanner/value.go Outdated Show resolved Hide resolved
spanner/value.go Outdated Show resolved Hide resolved
spanner/value.go Outdated Show resolved Hide resolved
spanner/value.go Outdated Show resolved Hide resolved
spanner/value.go Show resolved Hide resolved
Copy link
Contributor

@olavloite olavloite left a comment

LGTM (with a small nit on the documentation for the rounding option)

spanner/value.go Outdated Show resolved Hide resolved
spanner/value.go Outdated Show resolved Hide resolved
@hengfengli
Copy link
Contributor Author

hengfengli commented May 4, 2021

@skuruppu Can I get an approval from you? Otherwise, I can't merge this.

@hengfengli hengfengli merged commit cde8697 into googleapis:master Aug 23, 2021
4 checks passed
@hengfengli hengfengli deleted the add-invalid-numeric-error branch Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Cloud Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: invalid numeric should throw an error
3 participants