edgedb-rust icon indicating copy to clipboard operation
edgedb-rust copied to clipboard

`edgedb_protocol::model::Decimal` is largely unusable

Open jquesada2016 opened this issue 1 year ago • 1 comments

The Decimal type as provided is unusable without opting in to the with-bigdecimal feature flag. Without this, the type is completely opaque. Even if one does opt to using bigdecimal, it's an older version (0.3 rather than the current 0.4) so you get compilation errors when trying to convert a Decimal value into a BigDecimal value.

The two main improvements that can be done here are:

  1. Bumping the dependency version
  2. Adding crate-agnostic ways of getting the information needed in order to convert to another type, or at minimum, a Display implementation that can be used to parse into another type

jquesada2016 avatar Jul 27 '23 16:07 jquesada2016

Those are both valid proposals.

I'm not sure why we don't expose the underlying fields publicly. I'm certain that they are not going to change, since we are not going to change the data wire format for decimals. So there is no concern about backward compatibility.

That said, they are quite unpleasant to work with. They are documented here: https://docs.edgedb.com/database/reference/protocol/dataformats#std-decimal

aljazerzen avatar Apr 18 '24 15:04 aljazerzen

It might be worth adding a CI check that lets you guys know when dependencies are out of date. For example, bytes is a bit behind.

Edit: Also, it might be a good idea to relax version constraints. So depend on 1, rather than 1.5.9, for example.

jquesada2016 avatar Aug 04 '24 01:08 jquesada2016

@jquesada2016 versions in the Cargo.toml are not exact requirements, they are equivalent to carret semver requirement. Because we don't commit Cargo.lock, this means that even though we specify bytes version 1.0.1 in Cargo.toml, you can use these libraries and let cargo pick any version that is >=1.0.1 and <2.0.0.

I've just checked for any outdated dependencies and found only one, but even that one if a dev dependency.

aljazerzen avatar Aug 06 '24 11:08 aljazerzen

I suggest exposing those internal fields differently: #347

CodesInChaos avatar Aug 30 '24 08:08 CodesInChaos

I've just wrote that the API you proposed in #344 is better than just exposing the wire protocol. I take that back.

This intention of these fields is not to provide a great API for people to use, it is to just the data out with minimal performance implication, so they can then use it in what ever way they want.

The recommended way to use bitint and decimal is to use bigdecimal crate, which provides a great API for dealing with such numbers. If we want to provide a good API too, we should copy bigdecimal API into edgedb-rust. But that is very close to just exposing our bignumbers via bigdecimal!

So my conclusion is that it be best not to invent a new API here. Have the basic one (just wire protocol exposed that I've just added) and the two via bigdecimal and num-bigint crates.

aljazerzen avatar Aug 30 '24 15:08 aljazerzen