fix(spanner): invalid numeric should throw an error

[hengfengli]

Fix #3920

[olavloite]

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]

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

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

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]

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]

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]

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

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

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

[olavloite]

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

[hengfengli]

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