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

Golang big.Float is not a big decimal #203

Open
aaronc opened this issue Jan 13, 2023 · 12 comments
Open

Golang big.Float is not a big decimal #203

aaronc opened this issue Jan 13, 2023 · 12 comments
Labels
🐛 bug Defect / Bug

Comments

@aaronc
Copy link

aaronc commented Jan 13, 2023

🤔 What's the problem you've observed?

I am considering using the golang implementation of cucumber-expressions in https://github.com/regen-network/gocuke and noticed that bigdecimal is mapped to https://pkg.go.dev/math/big#Float.

The documentation for big.Float states that this is structured as sign × mantissa × 2**exponent. This is not a decimal number! It is a base-2 floating point number, not base-10 as required for decimals.

Java does provide a proper BigDecimal where the documentation clearly states that the structure is unscaledValue × 10-scale.

✨ Do you have a proposal for making it better?

The mapping of bigdecimal to big.Float should be removed in the golang package and consumers should be allowed to register a correct decimal implementation (none is provided by the standard library).

Also in the golang implementation, it seems like it's an error to override a built-in type mapping. It might be worth reconsidering this in case users want to swap out other type mappings.

📚 Any additional context?

Two correct golang decimal implementations are: https://pkg.go.dev/github.com/cockroachdb/apd/v3 and https://pkg.go.dev/github.com/ericlagergren/decimal/v3.


This text was originally generated from a template, then edited by hand. You can modify the template here.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jan 14, 2023

The mapping of bigdecimal to big.Float should be removed in the golang package and consumers should be allowed to register a correct decimal implementation (none is provided by the standard library).

That is perfectly acceptable. I'm kinda surprised that the big-decimal test case passed. I suppose we haven't quite added enough digits to it.

Would you be able to send pull request for this? Feel free to update the big decimal test case with enough digits to make it fail and then exclude the big decimal test case for Go. If other languages start failing we can look at those separately.

edit: I'm going to find a bigger consensus on this. I'd like to revert most if not all of #42.

Also in the golang implementation, it seems like it's an error to override a built-in type mapping. It might be worth reconsidering this in case users want to swap out other type mappings.

That sounds reasonable but I'd like to find a larger consensus.

@mpkorstanje
Copy link
Contributor

Note: I find the rationale from #42 to add {bigdecimal} to every language rather shaky. There don't appear to have been any other alternatives considered.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jan 14, 2023

The test case for big decimal also appears to be intentionally hamstrung to avoid the problems caused by a lack of precision. Compared to the big integer test case we're missing quite a few digits.

@aaronc
Copy link
Author

aaronc commented Jan 17, 2023

In the meantime should I submit a PR to disable the golang registration of bigdecimal?

@luke-hill
Copy link
Contributor

Ping @mpkorstanje - I support the ruby removal

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Feb 16, 2024

Alright, that is enough consensus for me (esp with #273 in mind). Let us remove the big decimal from Go and Ruby. It should be marked as a breaking change though so we don't blindside anyone.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Feb 16, 2024

@xeger @kieran-ryan for Javascript we currently have the same problem. Though going by #42 this code is also used in vscode somewhere, somehow, and removing it from Javascript would break that. Do you see a better, alternative solution on the vscode side?

@kieran-ryan kieran-ryan added the 🐛 bug Defect / Bug label Feb 19, 2024
@kieran-ryan
Copy link
Member

kieran-ryan commented Feb 19, 2024

Though going by #42 this code is also used in vscode somewhere, somehow, and removing it from Javascript would break that. Do you see a better, alternative solution on the vscode side?

Not sure I fully understand scope of changes just yet, however, bigdecimal is referenced in each language implementation of the language service - used by the language server and vscode extension. Perhaps there may not be a pressing need to remove from the language service in the near term - given users with versions up to now would be supported along with the subset of users if this change goes through? Though based on agreement above, we would need to remove at some stage, which I would be happy to support.

However, there may be changes required to support the following - though believe should be fine if registered as a Parameter Type in glue files or extension settings, would need to double check:

consumers should be allowed to register a correct decimal implementation (none is provided by the standard library)

@luke-hill
Copy link
Contributor

So basically we want to remove the BigDecimal pre-registered parameter type from several flavours of cucumber.

Any / all additional work for language server e.t.c. to not break it is also needed.

Reason -> Probably shouldn't have been added in first place, it's causing a few headaches now.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Feb 20, 2024

from several flavours of cucumber.

To be exact, all flavours that don't support a big decimal type.

Any / all additional work for language server e.t.c. to not break it is also needed.

Yes, but I'd like to consider those changes independently.

Though based on agreement above, we would need to remove at some stage, which I would be happy to support.

@kieran-ryan much appreciated!

@luke-hill
Copy link
Contributor

So ruby does support a bigdecimal type, but it involves in importing some rather large compiled libraries, for something that likely never gets used.

The way I look at it, if you want a bigdecimal type for your scenarios, import the library and define your own

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Feb 20, 2024

Mmh. I would consider those two scenarios to be equivalent for this discussion.

Unless you or someone else wants to spend sometime making ruby's implementation able to detect the presence of the big decimal library. And automatically add the type when the library is present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Defect / Bug
Projects
Status: No status
Development

No branches or pull requests

4 participants