Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#29823 closed Cleanup/optimization (fixed)

Incorrect Decimal handling in Sum queries

Reported by: Jurica Železnjak Owned by: Mariusz Felisiak <felisiak.mariusz@…>
Component: Documentation Version: 2.1
Severity: Normal Keywords:
Cc: Sergey Fedoseev Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Jurica Železnjak)

When doing Sum aggregate or annotate query on a DecimalField the result is a close-to-zero float (scientific notation) when it should be zero.

For example: 8 records, with following values: 7.85, -7, 19.85, -7, -5, -7, -0.85, -0.85
Aggregate Sum query on this column should equal Decimal('0.00'), but I'm getting Decimal('1.77635683940025E-15').

Before Django 2.1.0 this worked properly.

Change History (17)

comment:1 Changed 5 years ago by Jurica Železnjak

Description: modified (diff)

comment:2 Changed 5 years ago by Carlton Gibson

Before Django 2.1.0 this worked properly.

Any chance you could put that into a test case and bisect the regression?

comment:3 Changed 5 years ago by Jurica Železnjak

Minimal test case:

# models.py
from django.db import models


class TestModel(models.Model):
    decimal_field = models.DecimalField(max_digits=10, decimal_places=2)
# tests.py
from decimal import Decimal

from django.db.models import Sum
from django.test import TestCase

from decimal_test.models import TestModel


class DecimalTest(TestCase):
    @classmethod
    def setUpClass(cls):
        super().setUpClass()
        TestModel.objects.bulk_create([
            TestModel(decimal_field=7.85),
            TestModel(decimal_field=-7),
            TestModel(decimal_field=19.85),
            TestModel(decimal_field=-7),
            TestModel(decimal_field=-5),
            TestModel(decimal_field=-7),
            TestModel(decimal_field=-0.85),
            TestModel(decimal_field=-0.85)
        ])

    def testSum(self):
        sum = TestModel.objects.all().aggregate(sum=Sum('decimal_field'))['sum']
        print(sum)
        assert sum == 0, 'Should be zero'
        assert sum == Decimal(0.00)

FYI. Just tested with mysql and sqlite as db, same issue.

comment:4 Changed 5 years ago by Tim Graham

Cc: Sergey Fedoseev added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Bisected to ebc4ee3369694e6dca5cf216d4176bdefd930fd6. The test only fails on SQLite for me, not on MySQL, PostgreSQL, or Oracle.

comment:5 Changed 5 years ago by Simon Charette

Confirmed the test passes on MySQL and PostgreSQL for me as well.

comment:6 Changed 5 years ago by Claude Paroz

I think that this new behavior is correct and related to float-to-decimal conversion. The difference stems from the way you initialize the decimal fields. You should use strings (decimal_field='7.85') instead of floats (decimal_field=7.85).

For example:

>>> from decimal import Decimal
>>> Decimal(7.85)
Decimal('7.8499999999999996447286321199499070644378662109375')
>>> Decimal('7.85')
Decimal('7.85')
>>> 

comment:7 Changed 5 years ago by Tim Graham

I had the same thought, however, the issue persists after updating the test to use Decimals.

comment:8 Changed 5 years ago by Sergey Fedoseev

Owner: changed from nobody to Sergey Fedoseev
Status: newassigned

comment:9 Changed 5 years ago by Sergey Fedoseev

The problem is that SQLite doesn't really have decimal type, so I'm not sure if this issue can be really fixed. Even before ebc4ee3369694e6dca5cf216d4176bdefd930fd6 on SQLite you'll have this:

In [5]: vals = ['7.85', '-7', '19.85', '-7', '-5', '-7', '-0.85', '-0.85']

In [6]: DecimalModel.objects.values_list(sum(Value(Decimal(val), output_field=DecimalField()) for val in vals), flat=True).first()                                                                                  
Out[6]: Decimal('0.000000')

In [7]: DecimalModel.objects.values_list(sum(Value(Decimal(val), output_field=DecimalField()) for val in vals)*10**15, flat=True).first()                                                                           
Out[7]: Decimal('2.886580')

and this on PostgreSQL:

In [5]: DecimalModel.objects.values_list(sum(Value(Decimal(val), output_field=DecimalField()) for val in vals), flat=True).first()
Out[5]: Decimal('0.00')

In [6]: DecimalModel.objects.values_list(sum(Value(Decimal(val), output_field=DecimalField()) for val in vals)*10**15, flat=True).first()
Out[6]: Decimal('0.00')

So I'm open to any ideas on what can be improved here.

comment:10 Changed 5 years ago by Claude Paroz

Component: Database layer (models, ORM)Documentation
Severity: Release blockerNormal
Type: BugCleanup/optimization

The fact that SQLite has no real Decimal type and internally convert values to NUMERIC (8-byte IEEE floating point number) could be added to the database notes under https://docs.djangoproject.com/en/2.1/ref/databases/#sqlite-notes

Then https://docs.djangoproject.com/en/2.1/ref/models/fields/#decimalfield could contain a reference to that same note too.

In other words, if you care about decimal precision, you should NOT use SQLite.

comment:11 Changed 5 years ago by Jurica Železnjak

And what about testing? We generally use inmemory-sqlite as a database when running tests. Does that mean that we can't use it anymore?
Also... can't this just be forced when converting back to Python. Something like Decimal(whatever_sqlite_returns).quantize(Decimal('.01') (or whatever decimal places is applicable to the current field/column).

comment:12 Changed 4 years ago by Claude Paroz

Owner: Sergey Fedoseev deleted
Status: assignednew

Documentation PR.

comment:13 Changed 4 years ago by Claude Paroz

Has patch: set

comment:14 in reply to:  11 Changed 4 years ago by Nick Pope

Replying to Jurica Železnjak:

And what about testing? We generally use inmemory-sqlite as a database when running tests.

You should really be testing using the same database backend that you use in production.

Otherwise there are any number of subtle bugs that you could encounter such as this one.

comment:15 Changed 4 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Owner: set to Mariusz Felisiak <felisiak.mariusz@…>
Resolution: fixed
Status: newclosed

In b8dff52f:

Fixed #29823 -- Doc'd limitation of DecimalField on SQLite.

comment:16 Changed 4 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In c2057205:

[3.0.x] Fixed #29823 -- Doc'd limitation of DecimalField on SQLite.

Backport of b8dff52f440adfb78b40e19ee8bff45373ca2501 from master

comment:17 Changed 4 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In f452d42:

[2.2.x] Fixed #29823 -- Doc'd limitation of DecimalField on SQLite.

Backport of b8dff52f440adfb78b40e19ee8bff45373ca2501 from master

Note: See TracTickets for help on using tickets.
Back to Top