Opened 9 years ago

Closed 8 years ago

#5079 closed (fixed)

DecimalFields converted to float before being stored

Reported by: dlangdea@… Owned by: Karen Tracey
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Ian Kelly, richard.davies@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

If you create the following model:

class Bug(models.Model):
	field = models.DecimalField(max_digits=40, decimal_places=30)

The DecimalField is not being stored in the database correctly as the following code shows:

>>> b1 = Bug(field=Decimal(".1"))
>>> b1.save()
>>> b1 = Bug.objects.get(id=1)
>>> b1.field
Decimal("0.100000000000000005551115123126")

It seems to work correctly if the following change is made to db.models.fields.DecimalField.format_number by replacing

        return u"%.*f" % (self.decimal_places, value)

with

        if isinstance(value, decimal.Decimal):
        	return str(value.quantize(decimal.Decimal(".1")**self.decimal_places))
        else:
        	return u"%.*f" % (self.decimal_places, value)

I'm just starting out, so that fix might not be appropriate.

Attachments (4)

5079.diff (1.1 KB) - added by Philippe Raoult 9 years ago.
added regression tests for DecimalField
5079.2.diff (866 bytes) - added by Philippe Raoult 9 years ago.
added the correct regression tests (works for me though)
5079.3.diff (1.9 KB) - added by Philippe Raoult 9 years ago.
fix included now
5079_4.diff (2.3 KB) - added by Philippe Raoult 9 years ago.
better patch

Download all attachments as: .zip

Change History (30)

comment:1 Changed 9 years ago by John Shaffer <jshaffer2112@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

Unfortunately, that gives inconsistent results:

>>> unicode(Decimal("2500").quantize(Decimal(".1") ** -3))
u'2E+3'
>>> u"%.*f" % (-3, 2500)
u'2500'

If the value was converted to a string and then to a Decimal, or if the value was required to convert cleanly to a Decimal (which would make floats unusable, probably a bad idea), then it could work more consistently.

comment:2 Changed 9 years ago by John Shaffer <jshaffer2112@…>

Has patch: set
Patch needs improvement: set

comment:3 Changed 9 years ago by Manuel Saelices

Owner: changed from nobody to Manuel Saelices
Status: newassigned

comment:4 Changed 9 years ago by Philippe Raoult

Triage Stage: Design decision neededAccepted

comment:5 Changed 9 years ago by Philippe Raoult

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Works for me. I added a regression test though. I noticed while doing this that the DecimalField didn't enforce the decimal_places arguments itself, it takes a save() and get() from the DB to have the right value.

comment:6 Changed 9 years ago by Philippe Raoult

Summary: DecimalFields converted to float before being storedRegression test for models.DecimalField

Changed 9 years ago by Philippe Raoult

Attachment: 5079.diff added

added regression tests for DecimalField

comment:7 Changed 9 years ago by Philippe Raoult

Owner: changed from Manuel Saelices to Philippe Raoult
Status: assignednew

comment:8 Changed 9 years ago by Philippe Raoult

Status: newassigned

comment:9 Changed 9 years ago by John Shaffer

Summary: Regression test for models.DecimalFieldDecimalFields converted to float before being stored

PhiR: Please don't take over a ticket like that. Your test doesn't cover the original bug. Note the difference in precision.

When this is fixed, the whole num_chars part of DecimalField.format_number should either be removed or used somewhere. num_chars has existed, unused, since DecimalField was first added.

comment:10 Changed 9 years ago by John Shaffer

Has patch: unset
Triage Stage: Ready for checkinDesign decision needed

Changed 9 years ago by Philippe Raoult

Attachment: 5079.2.diff added

added the correct regression tests (works for me though)

comment:11 Changed 9 years ago by Philippe Raoult

Has patch: set

sorry for the botched patch. I did test the issue but then went on to test something else and submitted the wrong tests. 5079.2.diff shows that the bugs doesn't appear in SVN.

comment:12 Changed 9 years ago by John Shaffer

PhiR: Thanks for writing the test. It passes for me when using SQLite, but fails when using MySQL or PostgreSQL.

comment:13 Changed 9 years ago by Philippe Raoult

Triage Stage: Design decision neededReady for checkin

Maybe #6767 is related ?

Changed 9 years ago by Philippe Raoult

Attachment: 5079.3.diff added

fix included now

comment:14 Changed 9 years ago by Ian Kelly

This also fails when using Oracle, probably because the cx_Oracle driver returns a float rather than a Decimal object.

Please reduce the max_digits in the test case. Oracle can't handle anything greater than 38 (which is what I tested with).

comment:15 Changed 9 years ago by Ian Kelly

Cc: Ian Kelly added

comment:16 Changed 9 years ago by Ian Kelly

Also, I just noticed this patch doesn't seem to work when the decimal_places parameter > 28 (the precision in the default context). Is the decimal context supposed to be managed, or is the user expected to set the correct context as needed?

comment:17 Changed 9 years ago by Philippe Raoult

Triage Stage: Ready for checkinAccepted

I've noticed that too during my tests. I'll have a look at the Decimal doc. Note that it wasn't working before either, now it's just raising an exception instead of silently corrupting data :)

comment:18 Changed 9 years ago by Philippe Raoult

Patch needs improvement: set

The right thing would be to use string printing so we don't hit the precision limit. Then it would be the user's responsibility to make sure the decimal context matches his data. I'll try to update the patch.

Changed 9 years ago by Philippe Raoult

Attachment: 5079_4.diff added

better patch

comment:19 Changed 9 years ago by Philippe Raoult

Patch needs improvement: unset

Ok the new patch uses a private context so we do not mess with the global context.

comment:20 Changed 9 years ago by Philippe Raoult

Please note that the max_digits has been reduced so the test might run on oracle.

comment:21 Changed 9 years ago by Philippe Raoult

Triage Stage: AcceptedReady for checkin

comment:22 Changed 9 years ago by Ian Kelly

Well, the test still fails in Oracle, but that may be only because of #6767, so that's no reason to hold off on committing this.

comment:23 Changed 8 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This patch need some fixes before this can go in:

  1. Please put the tests in models.py (rather than tests.py), since that file already exists in that test directory and contains some other models.
  2. It's not at all clear from the test comments what you are actually trying to test here. What's all this stuff about "should fit in" (I can work out what it means, but it's not going to be clear to somebody reading it three months from now or who isn't deeply familiar with the decimal module or database storage constraints).
  3. Finally, John's comment number 9 seems to have been ignored, but it's quite valid: it looks like the num_chars stuff in _format() can be removed as part of this patch.

comment:24 Changed 8 years ago by Richard Davies <richard.davies@…>

Cc: richard.davies@… added

Don't know if this is exactly the same issue, but I had a similar problem specifically with the MySQL backend, and have fixed my issue with a patch to MySQLdb

http://sourceforge.net/tracker/index.php?func=detail&aid=2051833&group_id=22307&atid=374934

comment:25 Changed 8 years ago by Karen Tracey

Owner: changed from Philippe Raoult to Karen Tracey
Status: assignednew

#9565 reported this again. Since there's been no activity in this ticket lately I'm going to take a look at fixing up the patch here per Malcolm's notes.

comment:26 Changed 8 years ago by Karen Tracey

Resolution: fixed
Status: newclosed

(In [9394]) Fixed #5079 -- Avoid converting Decimals to floats during save to the database.

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