DBDecimal type loses precision when transferring both into and out of the database

[MasonD]

Module version(s) affected

6.*, present in previous versions but fixing would be a breaking change

Description

DBDecimal is the DBField type for fixed-precision DECIMAL SQL types. The PHP float type is not sufficient to store the values of such a type, as all database engines support precisions of DECIMAL much higher than float can store. This means we lose out on the precision of the DECIMAL SQL type.

How to reproduce

I'm using the default MySQLiConnector

Create a DataObject

class GlorifiedFloat extends DataObject {
    private static $table_name = 'GlorifiedFloat';
    private static $db = [
        'Value' => 'Decimal(20,18)'
    ];
}

try to store a precise value in the DataObject

$f = new GlorifiedFloat();
$f->Value = '0.99999999999999999';
$f->write();

$fromDB = GlorifiedFloat::get()->byID($f->ID);
echo $fromDB->Value;
echo "\n";  // 1

// interact with MySQL database engine directly
echo DB::prepared_query('SELECT CAST("Value" AS CHAR) FROM GlorifiedFloat WHERE ID = ?', [$f->ID])->value();
echo "\n";  // 1.000000000000000000

// directly set the value in the database to the correct value
DB::prepared_query('UPDATE GlorifiedFloat SET Value = \'0.99999999999999999\' WHERE ID = ?'. [$f->ID]);
echo DB::prepared_query('SELECT CAST("Value" AS CHAR) FROM GlorifiedFloat WHERE ID = ?', [$f->ID])->value();
echo "\n"; // 0.999999999999999990

$fromDBAgain = GlorifiedFloat::get()->byID($f->ID);
echo $fromDBAgain->Value;
echo "\n"; // 1

// And the MySQLiConnector will coerce a decimal to a float even outside of the DataObject
echo DB::prepared_query('SELECT "Value" FROM GlorifiedFloat WHERE ID = ?', [$f->ID])->value();
echo "\n"; // 1

Possible Solution

I'm pretty sure that fixing this would be a breaking change, but the only PHP-native representation suitable for SQL Decimals is string. The BCMath PHP extension is precedent for fixed-precision numbers represented as strings.

mysqli natively binds decimal type selects as strings, and MySQLiConnector goes out of its way to convert those to floats via MySQLStatement (https://github.com/silverstripe/silverstripe-framework/blob/6/src/ORM/Connect/MySQLStatement.php#L111). Just remove decimal from the types it coerces to float. I don't know how the other database connectors are written.

DBDecimal (and thus possibly DBCurrency and DBMoney) would have to be modified so that they use string representations. Possibly DBCurrency and DBMoney could still do conversion to float, but then DBCurrency would need to limit its precision argument (a 'Currency(20,2)' field would still be rounded when converted to float) so that it's within what a float can store.

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

[GuySartorelli]

@emteknetnz I think you're likely to have opinions on this given the DBField validation work you've been doing currently relies on DBDecimal explicitly not being represented by a PHP string

[emteknetnz]

What Guy's referring to is that DBDecimal is now validated by the new DecimalFieldValidator in CMS 6

DBDecimal currently uses float internally, and DecimalFieldValidator will validate the value is a numeric non-string, so either an int or a float. The int is allowed so that we're not failing validation if someone set the value using an int e.g. $myDBDecimal->setValue(3); which would be needlessly annoying

This makes the use of the Decimal type through the ORM effectively useless.

That seems a little harsh IMO, float seems appropriate, though only up to a point ~16 decimal places

php > var_dump(0.9999999999999999); // 16 dp
float(0.9999999999999999)
php > var_dump(0.99999999999999999); // 17 dp
float(1)

One possible solution here, though perhaps it's not great, is to just throw an exception during dev/build if more that 16 DP are specified by the column spec

[MasonD]

I guess I was a little harsh in my wording, sorry. What I meant by that is that, since every number stored in a DBDecimal field is converted to a float or int before and after being in the database, a DBDecimal's sole difference currently from a DBFloat is that it automatically rounds to a certain number of decimal digits

[emteknetnz]

There are some other differences, the main one being that DBDecimal will use the mysql decimal type, while DBFloat will use the mysql float type

Also, in CMS 6, at least in its current state, DBDecimal will have some actual validation on write(), so that oversized values (i.e. more precision than the DB column would allow) will no longer get "truncated" on write and will instead now throw a ValidationException. Meanwhile the validation on DBFloat is limited to simply checking that the value is numeric and not a string.

Still, your original point remains that floats are a limitation to the amount of precision that DBDecimal has

[MasonD]

The precision of float isn't quite 16 decimal digits. It's very close to 16 decimal digits if all precision is on the left side of the decimal point, but as a result of the floating point's behaviour, the fractional part can't store the same numbers as decimal can. For example,79071992547409.85 will become 79071992547409.84 or 550719925.4740935 becomes 550719925.4740936. So to avoid rounding from floating points we'd need to throw on total precision of 16 digits or higher.

[MasonD]

Ideally, there should probably be two Decimal field types. One with the convenience of float representation and one with extended precision but necessarily a string.

The problem I see with implementing that, though, is that it's the database connector that's responsible for type coercion when extracting from the database, and the only context it has to go on is the type the database driver reports for the column. For one type to be a string in a newly-hydrated DO while the other is a float, the connectors would need to return string uncoerced and it would have to be DataObject's responsibility to coerce into a float if needed. But DataObject's design is currently such that the record array isn't validated, just set directly as the row returned from the database connector.