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

Huge DECIMAL number casted to FLOAT/DOUBLE #13151

Closed
dmarkic opened this issue Nov 9, 2017 · 7 comments
Closed

Huge DECIMAL number casted to FLOAT/DOUBLE #13151

dmarkic opened this issue Nov 9, 2017 · 7 comments
Assignees
Labels
bug A bug report status: low Low
Milestone

Comments

@dmarkic
Copy link

dmarkic commented Nov 9, 2017

Expected and Actual Behavior

Using bcmath to calculate few things and than storing it into MySQL DECIMAL(30,20) column will cast the number to double/float which causes the loss of decimal places.

Script to reproduce the issue

<?php
/**
 * Phalcon decimal bug (cast to float)
 */
// calculated value
$value = '0.12345678901234567890';

class ModelWithDecimal extends \Phalcon\Mvc\Model
{
    public $id;
    public $value;
}

$di = new \Phalcon\Di\FactoryDefault();

$di->set('db', function () use ($di) {
    return new \Phalcon\Db\Adapter\Pdo\Mysql(array(
        'host'      => 'localhost',
        'username'  => 'test',
        'password'  => 'test',
        'dbname'    => 'test'
    ));
});


$M = new ModelWithDecimal;
$M->value = $value;
$M->save();

echo " value: " . $value . "\n";
echo " M->value: " . $M->value . "\n";

$M = ModelWithDecimal::findFirst(1);
echo " load: M->value: " . $M->value . "\n";

Output

# ./decimal.php 
 value: 0.12345678901234567890
 M->value: 0.12345678901234567890
 load: M->value: 0.12345678901235000000
#

Expected output

# ./decimal.php 
 value: 0.12345678901234567890
 M->value: 0.12345678901234567890
 load: M->value: 0.12345678901234567890
#

Analysis

As far as I could I've traced this cast to this code:

let castValue = doubleval(value),

Solution

I've extended the \Phalcon\Db\Adapter\Pdo\Mysql class and modified this a bit like this:

use \Phalcon\Db\Column;

class Mysql extends \Phalcon\Db\Adapter\Pdo\Mysql
{
 
    /**
     * Phalcon tries to convert DECIMAL to doubleval() which
     * looses the decimal precision. We change the bind type to string.
     */
    public function executePrepared(\PDOStatement $statement, array $placeholders, $dataTypes) : \PDOStatement
    {
        foreach ($placeholders as $wildcard => $value) {
            if (is_numeric($wildcard)) {
                $parameter = $wildcard + 1;
            } elseif (is_string($wildcard)) {
                $parameter = $wildcard;
            } else {
                throw new \Exception('Invalid bind parameter (me)');
            }
            if (is_array($dataTypes)) {
                $type = ($dataTypes[$wildcard] ?? null);
                if ($type !== null) {
                    if ($type == Column::BIND_PARAM_DECIMAL) {
                        $dataTypes[$wildcard] = Column::BIND_PARAM_STR;
                    }
                }
            }
        }
        return parent::executePrepared($statement, $placeholders, $dataTypes);
    }
}

This does solve my problem, but I guess it could be solved some other way?

Details

  • Phalcon version: (php --ri phalcon)
phalcon


Web framework delivered as a C-extension for PHP
phalcon => enabled
Author => Phalcon Team and contributors
Version => 3.2.4
Build Date => Oct 21 2017 16:34:03
Powered by Zephir => Version 0.10.4-11e39849b0

Directive => Local Value => Master Value
phalcon.db.escape_identifiers => On => On
phalcon.db.force_casting => Off => Off
phalcon.orm.events => On => On
phalcon.orm.virtual_foreign_keys => On => On
phalcon.orm.column_renaming => On => On
phalcon.orm.not_null_validations => On => On
phalcon.orm.exception_on_failed_save => Off => Off
phalcon.orm.enable_literals => On => On
phalcon.orm.late_state_binding => Off => Off
phalcon.orm.enable_implicit_joins => On => On
phalcon.orm.cast_on_hydrate => Off => Off
phalcon.orm.ignore_unknown_columns => Off => Off
phalcon.orm.update_snapshot_on_save => On => On
phalcon.orm.disable_assign_setters => Off => Off
  • PHP Version: (php -v)
PHP 7.0.19-1 (cli) (built: May 11 2017 14:04:47) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.0.19-1, Copyright (c) 1999-2017, by Zend Technologies
    with Xdebug v2.5.0, Copyright (c) 2002-2016, by Derick Rethans
  • Operating System: Linux debian stretch
  • Installation type: installing via package manager: https://packagecloud.io/phalcon/stable/debian/ stretch main
  • Zephir version (if any):
  • Server: Other
  • Other related info (Database, table schema):
CREATE TABLE `model_with_decimal` (
	`id` INT(10) UNSIGNED NOT NULL AUTO_INCREMENT,
	`value` DECIMAL(30,20) UNSIGNED NOT NULL,
	PRIMARY KEY (`id`)
)
COLLATE='utf8_general_ci'
ENGINE=InnoDB
@stale
Copy link

stale bot commented Apr 16, 2018

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

@stale stale bot added the stale Stale issue - automatically closed label Apr 16, 2018
@scrnjakovic
Copy link
Contributor

scrnjakovic commented Oct 28, 2018

@sergeyklay @niden this shouldn't be closed as it is still a valid issue. As PHP does not support decimals natively, DECIMAL columns should be casted to STRING. Casting to double/float introduces a fresh hell of imprecisions. Saving longitude (20.44892159999995) this way introduces a lot of issues.

All in all, MySQL's DECIMAL(M,D) does not equal to PHP's double, but string (unfortunately). MySQL's FLOAT and DOUBLE on the other hand, are good candidates for PHP's double

@niden niden reopened this Oct 28, 2018
@stale stale bot removed the stale Stale issue - automatically closed label Oct 28, 2018
@niden
Copy link
Member

niden commented Oct 28, 2018

Thanks @scrnjakovic

This needs a bit more discussion actually on how we can address this. Decimal should be a double. However in the example above, keeping it as double will not help.

I am wondering if this can be a user initiated change i.e. where the column is defined as string/varchar.

@scrnjakovic
Copy link
Contributor

scrnjakovic commented Oct 28, 2018

Thanks @niden for reopening this :)

Decimal should be a double.

I would have to disagree with this. Unlike float and double, decimal has fixed precision.

PHP float double docs:

Floating point numbers have limited precision. Although it depends on the system, PHP typically uses the IEEE 754 double precision format, which will give a maximum relative error due to rounding in the order of 1.11e-16. Non elementary arithmetic operations may give larger errors, and, of course, error propagation must be considered when several operations are compounded.

MySQL FLOAT and DOUBLE docs:

Because floating-point values are approximate and not stored as exact values, attempts to treat them as exact in comparisons may lead to problems. They are also subject to platform or implementation dependencies.

MySQL DECIMAL and NUMERIC docs:

The DECIMAL and NUMERIC types store exact numeric data values. These types are used when it is important to preserve exact precision, for example with monetary data.

From this it's very clear that column with DECIMAL data type must not be treated as double in PHP, otherwise we are treating something that is meant and expected to be precise as something that is by definition not precise.

@niden
Copy link
Member

niden commented Oct 28, 2018

I see your point @scrnjakovic. I have no problem switching this to a bind type of string which will not force double on the value and thus create all sorts of issues as you mentioned above.

I think that we can get away with doing this provided that it is crystal clear in the documentation that if one uses decimal they should expect a string.

@niden
Copy link
Member

niden commented Oct 28, 2018

@niden niden self-assigned this Oct 28, 2018
@niden niden added this to the 4.0.0 milestone Oct 28, 2018
@niden
Copy link
Member

niden commented Oct 31, 2018

Addressed in #13562

@niden niden closed this as completed Oct 31, 2018
@niden niden added the 4.0 label Jun 21, 2019
@niden niden added bug A bug report status: low Low and removed Bug - Low labels Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report status: low Low
Projects
None yet
Development

No branches or pull requests

4 participants