Opened 8 years ago

Closed 7 years ago

#5079 closed (fixed)

DecimalFields converted to float before being stored

Reported by: dlangdea@… Owned by: kmtracey
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: ikelly, 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 PhiR 8 years ago.
added regression tests for DecimalField
5079.2.diff (866 bytes) - added by PhiR 8 years ago.
added the correct regression tests (works for me though)
5079.3.diff (1.9 KB) - added by PhiR 7 years ago.
fix included now
5079_4.diff (2.3 KB) - added by PhiR 7 years ago.
better patch

Download all attachments as: .zip

Change History (30)

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 8 years ago by John Shaffer <jshaffer2112@…>

  • Has patch set
  • Patch needs improvement set

comment:3 Changed 8 years ago by msaelices

  • Owner changed from nobody to msaelices
  • Status changed from new to assigned

comment:4 Changed 8 years ago by PhiR

  • Triage Stage changed from Design decision needed to Accepted

comment:5 Changed 8 years ago by PhiR

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready 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 8 years ago by PhiR

  • Summary changed from DecimalFields converted to float before being stored to Regression test for models.DecimalField

Changed 8 years ago by PhiR

added regression tests for DecimalField

comment:7 Changed 8 years ago by PhiR

  • Owner changed from msaelices to PhiR
  • Status changed from assigned to new

comment:8 Changed 8 years ago by PhiR

  • Status changed from new to assigned

comment:9 Changed 8 years ago by jshaffer

  • Summary changed from Regression test for models.DecimalField to DecimalFields 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 8 years ago by jshaffer

  • Has patch unset
  • Triage Stage changed from Ready for checkin to Design decision needed

Changed 8 years ago by PhiR

added the correct regression tests (works for me though)

comment:11 Changed 8 years ago by PhiR

  • 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 8 years ago by jshaffer

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

comment:13 Changed 7 years ago by PhiR

  • Triage Stage changed from Design decision needed to Ready for checkin

Maybe #6767 is related ?

Changed 7 years ago by PhiR

fix included now

comment:14 Changed 7 years ago by ikelly

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 7 years ago by ikelly

  • Cc ikelly added

comment:16 Changed 7 years ago by ikelly

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 7 years ago by PhiR

  • Triage Stage changed from Ready for checkin to Accepted

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 7 years ago by PhiR

  • 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 7 years ago by PhiR

better patch

comment:19 Changed 7 years ago by PhiR

  • Patch needs improvement unset

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

comment:20 Changed 7 years ago by PhiR

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

comment:21 Changed 7 years ago by PhiR

  • Triage Stage changed from Accepted to Ready for checkin

comment:22 Changed 7 years ago by ikelly

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 7 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 7 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 7 years ago by kmtracey

  • Owner changed from PhiR to kmtracey
  • Status changed from assigned to new

#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 7 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from new to closed

(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