-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
proposal: database/sql: define a Decimal decompose interface for decimal packages #30870
Comments
I think I really like this idea. I’ll have more feedback later. |
The immediate problem I see with this is the non-numeric decimal representations. apd and ericlagergren's decimal type support positive and negative infinity, nan, and signaling nan. The shopspring decimal and many others do only mantissa + exponent. The special values are handled well with floats because they are defined as part of the bits. I think that if we want adoption of this interface it would need to support more than finite numbers. |
Just to clarify... |
@renthraysk Yes. It would first attempt to match the type (like apd.Decimal -> apd.Decimal) but failing that, if both the src and dest satisfy the DecimalValue type, it could extract the bits from one and stuff it into the other. |
Leaves NULL handing. type NullDecimal struct { ? |
@ericlagergren If you have time this week, additional feedback would be appreciated. @mjibson What do you think of the edited interface? I modified the interface to take a "form", as well as the the set method returning a method. This way if a decimal implementation and the number was too large, or the implementation doesn't support NaN or infinity and the number was in that form, it could return an error. |
I recommend removing pos and neg infinity and just having infinity. The sign of the infinity can be extracted from the sign of the big.Int value. In addition, I recommend adding Separating the sign from the form is important because in addition to +/- infinity, the spec also supports +/- NaN and +/- sNaN. See http://speleotrove.com/decimal/damodel.html, which discusses the requirements of finite and special values, and is linked to by the python decimal module. Although there is a very good argument to be made that the forms you have as of now (pos inf, neg inf, nan, finite) are sufficient since the sign and type of nan almost never matters, and the java bigdecimal implementation only supports those forms. apd supports 4 kinds of nan, but I suspect most users only use the normal nan. In our use of apd, we certainly only use the normal nan and coerce all other nans to it. |
@mjibson Thanks for the feedback. I appreciate the links. I finished reading the speletrove link and skimmed the python doc link. My point of reference is mostly line of business applications. The implementations I'm looking at are C#, Java, Oracle, PostgreSQL, and MS SQL Server.
Is there a different set of use cases I'm not thinking of where transmitting the signed NaN, or various forms of NaN would be advantageous? |
Honestly no, I'm not aware of anyone caring about any kind of NaN other than just the normal NaN. I guess in that light I think your current proposal is fine. I almost think the previous one with posinf, neginf, nan, finite may be better just because it feels medium risky to tell people to inspect the sign of the big.Int. Unsure. |
CC @griesemer |
@mjibson When I wrote some pseudo code to unmarshal and marshal the infinity forms, it was clearer to keep Positive and Negative Infinity as separate forms. I reverted to having them separate. |
Overall, I like the idea. I'm kind of torn between including different types of NaN and infinity values, though. I don't see much value in inserting That being said, if people do need those values it'll be difficult to add them on top of the aforementioned So, a bitmask seems like a decent approach:
Something else not considered is |
Additionally, I know decimals are well-defined, but I would take the extra step to add The reason for this is some libraries like my If the equation is defined in the |
math/big doesn't support NaN, so there's no point in having it in such an API. More generally, conversion from/to decimals requires parsing/printing the decimal numbers; in other words these are string conversions. The conversions in each of these directions is a handful of lines of code. I am not convinced that these need to be added to the big.Float API. See here for an example implementation: https://play.golang.org/p/iJpjp9J11yy The code is easily adjusted to provide big.Ints if that is desired. The string ops may be not totally optimal but negligible (in terms of overhead) compared to the actual Float conversions (which are expensive, and there is no way around those). If the decimals are in []byte rather than in string form, then one can use Un/MarshalText instead. In short, it seems to me that it's better to write the few lines of code that are custom-fitted for the specific application in mind than adding more API that may not be quite right in the specific case. |
This is about getting decimals (including NaNs) in and out of database/sql and drivers without having to depend on any specific implementation. |
@renthraysk NaNs are not "decimals". There's no support for NaNs in math/big by design (and we're not going to add support for NaNs); so let's leave them out of the picture. If you need to represent NaNs, you can always have a special encoding outside of math/big. I don't know what you mean by "without relying on a specific implementation". The title of this issue is pretty clear, the specific implementation is math/big. Please clarify. |
I hope I'm not being too pedantic or misunderstanding your point, but NaN values are decimals in that they're special values explicitly covered by decimal floating point specs. |
@griesemer Thank you for looking at this issue. I mostly focus on I would be nice to have a base-10 decimal type in the standard library, but baring that, it would be nice to have an interface for marshaling and unmarshaling decimal types in and out of non-standard types. This interface could be defined in "math/big", or it could be implicitly defined in decimal packages as we mutually agree to it. So while this could involve "math/big", the interface could also be implicit. Example 1:
Example 2:
Let's ignore NaN at the moment. Regardless, as a database driver, telling clients to always go through strings or a []byte interface isn't really an option I see as viable. I hope this clears up what I'm trying to accomplish with this issue. |
@ericlagergren Point taken, but again, they are not supported by math/big, by design (math/big is a software library, not a hardware standard, and as such has many other ways to express errors such as a division by zero w/o resorting to special "in-band" values such as NaNs). @kardianos Thanks for the clarification. I suggest you change the title of this issue to something that better reflects the intent. From you response I gather that this issue is only marginally related to math/big. |
@griesemer |
@kardianos Thanks for the title change, this makes is much clearer (to me, at least). I'm somewhat amazed that all the Decimal packages are using a type Decimal interface {
// Value returns the sign, mantissa, and scale factor of the decimal value x, such that
// x = sign * mant * 10^exp, where a sign < 0 indicates a negative value (otherwise it
// is a positive value), mant contains the mantissa bits in little-endian binary format,
// and scale is the scale factor by which the mantissa is adjusted. The value 0 is
// represented by an empty mantissa (len(mant) == 0) and a scale of zero, while an
// infinity uses an empty mantissa with any non-zero scale value. If a sufficiently
// large buf is provided, its underlying array is used for mant.
Value(buf []byte) (sign int, mant []byte, scale int)
// ...
SetValue(sign int, mant []byte, scale int) error
} With an interface like this one wouldn't be so tied to Some questions to answer:
Also, how important is it to be able to represent NaNs? |
@griesemer Thank you for the continued feedback. It would be nice to not be coupled to
|
@kardianos Please find my replies below, in the order of your questions:
|
I haven't fully digested your last two comments yet, but I'm curious what you meant by
|
Decimals (or at least the decimal spec previously mentioned) have the ability to differentiate between representations like |
@ericlagergren What I meant is literally that I am surprised that given six essentially independent packages (I assume that's the case) all chose a very similar implementation. I would have expected that some of them would use a custom, fixed-size representation. |
When the decomposer interface was added (though not yet publicly declared), it was designed to gracefully grow into it. https://github.com/golang-sql/decomposer I failed to take into account one scenario: Should a decimal type directly implement a driver.Valuer interface (like shopspring) or should it be embedded into a decimal type that satisfies a Valuer interface (like sqlboiler does), the driver.DefaultParameterConverter will recognize the value as a valid value because IsValid was modified to recognize the decomposer value. This combination breaks existing implementations when the sql driver doesn't recognize the decomposer interface. I can either try to rollback the decomposer experiment, either partially or fully, or we can recognize the decomposer type as a fundamental type that may get passed to the driver. I would prefer to update database drivers to support the decomposer interface, because I see this as the best way forward to support decimals generally. |
I'd happily add support to the sql driver. |
I vote for this option. |
@kardianos I suspect there is a bug in database/sql/convert.go: 683ffe0#r45892984 |
@dolmen Can you look at this playground snip and tell me what I'm doing wrong? |
With this package, everything is fine |
Also, handle decimalDecompose types for golang/go#30870
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
It seems to me that this proposal has been implemented and support has been merged; yet the issue is Open and the proposal is put on hold given the issues discovered... ...does that mean that it can still be rolled back in a future version of go, and that code using the feature could suddenly break in a go upgrade? Or is it OK to build on this? |
@dagss The proposed |
See
So it appears to be included, at least in my go1.18.1 install. It doesn't define a public interface, but it defines a a private interfaces The question is if it could be removed again... |
We aren't going to roll back that change. @kardianos Is there anything else to do here? Thanks. |
@ianlancetaylor @kardianos what approach should we take for go-mssqldb in our Microsoft fork to support decimals, based on everything discussed here? I want to make sure our sqlcmd implementation can handle decimals and money, so if it means taking an expedient path and just handling them as strings at the driver for now I'll probably do it. |
Return strings for noe, but implement the decimal(De)Composer methods as a performance optimization route. |
Just to clarify the E2E desired behavior of a driver like go-mssqldb now: Instead of reporting For supporting input parameters of a decimal type, the driver will need to define its own local copies of Does that sum it up? |
I believe this comment seems erroneous: go/src/database/sql/driver/types.go Lines 253 to 256 in 40cdf69
A few lines before this function does: go/src/database/sql/driver/types.go Lines 238 to 240 in 40cdf69
And go/src/database/sql/driver/types.go Lines 176 to 187 in 40cdf69
|
Background
Databases store many types of values. One type of very common value databases store, especially SQL databases, are decimal values. Some databases limit the size of the decimal, while others support arbitrary precision. But common to all is a base10 representation of the decimal number.
Historically handling decimals for
database/sql
and database drivers has been a pain. What types should a driver look for? If a driver looks for a given type and handles it, then it has to import it and depend on it. Or possibly go to the trouble of injecting the various types it handles with some type of type registry. The solution space at present for "library" packages that need to deal with decimals, but may be used by many other applications, is sub-optimal.There is a history of proposals for including a decimal type into the standard library:
#19787 #12127 #12332 .
Lastly, there are a number of decimal packages. Each implementation has a similar type implementation:
Generally each decimal type has a
big.Int
coefficient and anint32
exponent. All of these types of decimals are arbitrary precision, not fixed size likedecimal128
.Proposal
I propose that a common interface is defined for a decimal type that dumps the
big.Int
and int32 exponent. This interface could be defined in the standard library, or agreed upon by each package. This way a SQL driver would need to do a type assertion for the well known interface, dump the value and exponent, and marshal the value to the driver. When scanning a decimal value, it would try to load the value and exponent into the decimal type presented to it.It may be necessary to also provide some test vectors along with the implementation so the value and exponent are interpreted the same.
The proposed interface:
This is the original proposed interface: out-dated, see this comment or see above.
/cc @mjibson @victorquinn @ericlagergren
If this, or a similar, interface is acceptable to cockroachdb, shopspring, and Eric, I'd be willing to open up PRs to add the methods and some test vectors.
Edited proposed interface to include a "form".
The text was updated successfully, but these errors were encountered: