Opened 18 years ago

Closed 14 years ago

Last modified 12 years ago

#399 closed enhancement (fixed)

Bigint field object needed

Reported by: jmadson@… Owned by: Tomáš Kopeček
Component: Database layer (models, ORM) Version: dev
Severity: normal Keywords: sprintsept14, bigint
Cc: listuser@…, richard.davies@…, gabor@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The existing IntegerField is not adequate for generating schemas that require a "bigint" field. Either IntegerField needs to take a size/length parameter or a new Field type is necessary.

Attachments (20)

bigint_patch.txt (7.1 KB) - added by MattCroydon 18 years ago.
bigint_patch_06aug06.txt (4.5 KB) - added by Gopal Narayanan <gopastro@…> 17 years ago.
bigint_patch_03jan07.diff (4.4 KB) - added by Gaël Chardon <gael.dev@…> 17 years ago.
399.diff (5.1 KB) - added by Marc Fargas <telenieko@…> 16 years ago.
Last patch with docs added.
django-bigint-20070711.patch (7.0 KB) - added by Peter Nixon <listuser@…> 16 years ago.
Add BigIntegerField support to django (SQLite, MySQL, Postgresql, Oracle, MSSQL)
django-bigint-20070712.patch (7.4 KB) - added by Peter Nixon <listuser@…> 16 years ago.
Add BigIntegerField? support to django (SQLite, MySQL, Postgresql (and postgresql_psycopg2), Oracle, MSSQL)
patch_tests.diff (8.8 KB) - added by Tomáš Kopeček 16 years ago.
Previous patch with simple tests
patch.diff (9.2 KB) - added by Tomáš Kopeček 16 years ago.
Updated, added constraint checking
bigint-patch-2007-09-15.diff (9.2 KB) - added by Tomáš Kopeček 16 years ago.
bigint-patch-2008-01-04.diff (9.2 KB) - added by Tomáš Kopeček 16 years ago.
English typos
bigint-patch-2008-02-03.diff (7.8 KB) - added by Tomáš Kopeček 16 years ago.
-1 problem fix
bigint-patch-2008-04-13.diff (6.8 KB) - added by cpinto 15 years ago.
bigint-patch-2008-04-25.diff (8.7 KB) - added by mwdiers <martin@…> 15 years ago.
Restored tests to previously submitted patch
bigint-patch-2008-06-28.diff (7.4 KB) - added by Tomáš Kopeček 15 years ago.
missing typecast
bigint-patch-2009-10-26.diff (7.6 KB) - added by Tomáš Kopeček 14 years ago.
updated to trunk
bigint-patch-2009-11-23.diff (9.8 KB) - added by Tomáš Kopeček 14 years ago.
updated patch, modelform, tests are back (removed typo in comments template)
bigint-patch-2009-12-07.diff (9.9 KB) - added by Tomáš Kopeček 14 years ago.
updated patch (typo, tests)
bigint-patch-2009-12-09.diff (12.0 KB) - added by Ian Kelly 14 years ago.
Added Oracle introspection and test
bigint-patch-20091215.diff (12.6 KB) - added by Tomáš Kopeček 14 years ago.
fixed: correct saving null fields + test
kmt-bigint.diff (14.2 KB) - added by Karen Tracey 14 years ago.

Download all attachments as: .zip

Change History (93)

comment:1 Changed 18 years ago by jmadson@…

Additionally, a signedness boolean option is needed on IntegerField.

Changed 18 years ago by MattCroydon

Attachment: bigint_patch.txt added

comment:2 Changed 18 years ago by MattCroydon

See bigint_patch.txt for a BigIntegerField and PositiveBigIntegerField. I took it for a spin on mysql but it needs testing on postgres and sqlite (the backend-specific stuff is there but has not been tested).

jmadson: I think PositiveIntegerField, SmallPositiveIntegerField, or BigPostitiveIntegerField might be what you're looking for unless I misunderstood.

comment:3 Changed 18 years ago by MattCroydon

BigIntegerField seems OK on PostgreSQL, but PostitiveBigIntegerField throws ProgrammingError: ERROR: integer out of range for 18446744073709551615. I'll see what I can do.

comment:4 Changed 18 years ago by MattCroydon

See my post for details, but it looks like a PostitiveBigInteger on PostgreSQL is only going to reach 9 bajillion whereas MySQL does 18 bajillion. Still needs testing on sqlite.

Changed 17 years ago by Gopal Narayanan <gopastro@…>

Attachment: bigint_patch_06aug06.txt added

comment:5 Changed 17 years ago by Gopal Narayanan <gopastro@…>

Matt Croydon patch is now quite dated for the existing codebase. I have updated ticket #399 with the new patch. It is called bigint_patch_06aug06.txt. Since I am most familiar with MySQL, I modified the backend code for MySQL. The changes for Oracle, postgres, sqlite, etc. should be minimal, and should be done for django/db/backends.
I have done some simple tests. Please take it out for a spin.

comment:6 Changed 17 years ago by anonymous

Summary: Bigint field object needed[patch] Bigint field object needed

Changed 17 years ago by Gaël Chardon <gael.dev@…>

Attachment: bigint_patch_03jan07.diff added

comment:7 Changed 17 years ago by Gaël Chardon <gael.dev@…>

I have updated Gopal (previously Matt Croydon) patch against revision [4274]
See bigint_patch_03jan07.diff

comment:8 Changed 17 years ago by Chris Beaven

Component: MetasystemDatabase wrapper
Triage Stage: UnreviewedDesign decision needed

comment:9 Changed 17 years ago by Chris Beaven

Needs documentation: set
Patch needs improvement: set
Triage Stage: Design decision neededAccepted

Ok, on second thoughts this is a valid ticket.

Current patch needs some documentation and implementation for other database backends.

Changed 16 years ago by Marc Fargas <telenieko@…>

Attachment: 399.diff added

Last patch with docs added.

comment:10 Changed 16 years ago by Marc Fargas <telenieko@…>

Needs documentation: unset
Summary: [patch] Bigint field object neededBigint field object needed

needs_docs = 0 and stripping redundant prefix from summary.

Changed 16 years ago by Peter Nixon <listuser@…>

Add BigIntegerField support to django (SQLite, MySQL, Postgresql, Oracle, MSSQL)

comment:11 Changed 16 years ago by Jacob

Needs tests: set

Changed 16 years ago by Peter Nixon <listuser@…>

Add BigIntegerField? support to django (SQLite, MySQL, Postgresql (and postgresql_psycopg2), Oracle, MSSQL)

comment:12 Changed 16 years ago by Peter Nixon <listuser@…>

Oops. I missed introspection support for postgresql_psycopg2. Updated patch attached.

comment:13 Changed 16 years ago by Peter Nixon <listuser@…>

Cc: listuser@… added

comment:14 Changed 16 years ago by Thomas Güttler <hv@…>

The patch django-bigint-20070712.patch is working for me (psycopg2)

Changed 16 years ago by Tomáš Kopeček

Attachment: patch_tests.diff added

Previous patch with simple tests

Changed 16 years ago by Tomáš Kopeček

Attachment: patch.diff added

Updated, added constraint checking

comment:15 Changed 16 years ago by Tomáš Kopeček

Needs tests: unset
Owner: changed from nobody to Tomáš Kopeček
Patch needs improvement: unset
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:16 Changed 16 years ago by Tomáš Kopeček

Keywords: sprintsept14 added

comment:17 Changed 16 years ago by Matt McClanahan

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

The latest patch's approach for constraint checking (sys.maxint) isn't going to work for 32bit systems.

Changed 16 years ago by Tomáš Kopeček

comment:18 Changed 16 years ago by Tomáš Kopeček

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

sys.maxint was changed to class constant.

comment:19 Changed 16 years ago by Jacob

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This isn't yet ready for checkin. The MAX_BIGINT variable in django.db.models.fields is fixed, but in reality is dependant on both architecture and database backend. I'm not sure what the fix is, but hard-coding 64 bits is going to look as silly in ten years as hardocoding 32 bits does today.

comment:20 in reply to:  19 Changed 16 years ago by Tomáš Kopeček

Replying to jacob:

This isn't yet ready for checkin. The MAX_BIGINT variable in django.db.models.fields is fixed, but in reality is dependant on both architecture and database backend. I'm not sure what the fix is, but hard-coding 64 bits is going to look as silly in ten years as hardcoding 32 bits does today.

But we _define_ BIGINT as 64 bits long (as written in doc). So why not to hardcode it that it will be always 64bit? Also in all db backends it is defined as 64bit type. So, there is no same situation like with integer data type which has always denpended on size of CPU register. This will always be 64bits. If there is a variant that db backend cannot save 64bit value, then we cannot us bigint type at all. But all backends supports 64bit values, so there is no problem with this. Yes, I also don't like `magic' value, but I think that it is adequate here because python doesn't define anything similar to sys.maxint.

comment:21 Changed 16 years ago by Vitaliy <vitaliyf@…>

Typo in latest patch:

raise ValueError("Value is so small/large to fit to field")

should be

raise ValueError("Value is too small/large to fit this field")

(or something like that. Also, same message is in tests/modeltests/field_types/models.py)

comment:22 Changed 16 years ago by corporeal

The SQL specification defines bigint as a 64-bit signed integer, and as such the definition of bigint in the context of SQL will never change, just as the definition of int has not changed despite the need for 64-bit signed integers today. Thus I believe jacob's reason for delaying check-in of this patch to be invalid. Please change check this in as soon as possible.

comment:23 Changed 16 years ago by Aaron Krill <corporeal@…>

Triage Stage: AcceptedReady for checkin

comment:24 Changed 16 years ago by James Bennett

Triage Stage: Ready for checkinAccepted

"Ready for checkin" is a determination made by triagers and committers. Please don't set it unless you're one of those people.

Changed 16 years ago by Tomáš Kopeček

English typos

comment:25 in reply to:  22 Changed 16 years ago by Tomáš Kopeček

Triage Stage: AcceptedReady for checkin

Replying to corporeal:
Agreed, changing to 'ready for checkin'

comment:26 Changed 16 years ago by Ramiro Morales

Triage Stage: Ready for checkinAccepted

comment:27 in reply to:  26 Changed 16 years ago by Tomáš Kopeček

Replying to ramiro:

Please, please read http://code.djangoproject.com/ticket/399#comment:24 and http://www.djangoproject.com/documentation/contributing/#ticket-triage

Ticket triagers: community members who keep track of tickets, making sure the tickets are always categorized correctly.

Probably I don't understand this correctly. Who are triagers if not us? Is there any list of them? How to make some progress with tickets, whose are ready for months and no one moves them to next stage?

comment:28 Changed 16 years ago by Marc Fargas

Keywords: bigint added
Patch needs improvement: unset
Version: SVN

Recalling comments, to avoid triagers loosing time reading the full log to take conclusions:

  • In comment:20 jacob says that why would we hardcode that Bigint field to 64bits. Thus marking as "needs improvement".
  • But corporeal says: "The SQL specification defines bigint as a 64-bit signed integer"

So, hardcoding it is not that bad. maybe we could ask the backend about the bigint size, but writting this for every backend knowing that BIGINT must be 64bits long seems a bit cumbersome.

So then, I'm setting "needs improvement" to 0, any triager please move to "Ready for checkin" or "Please ask every backend if they feel right beeing 64bit long" :)

comment:29 Changed 16 years ago by honeyman

Sorry for meddling in, but shouldn't django/db/models/fields/init.py have

if value > self.MAX_BIGINT or value < -self.MAX_BIGINT - 1

instead of

if value > self.MAX_BIGINT or value < -self.MAX_BIGINT

?

comment:30 in reply to:  29 Changed 16 years ago by Tomáš Kopeček

Replying to honeyman:

Sorry for meddling in, but shouldn't django/db/models/fields/init.py have

if value > self.MAX_BIGINT or value < -self.MAX_BIGINT - 1

instead of

if value > self.MAX_BIGINT or value < -self.MAX_BIGINT

?

You're right. Appending new patch.

Changed 16 years ago by Tomáš Kopeček

-1 problem fix

Changed 15 years ago by cpinto

comment:31 Changed 15 years ago by cpinto

The patch I just submitted fixes an "integer out of range" in pgsql by specifying the internal type of the BigIntegerField

comment:32 in reply to:  description Changed 15 years ago by mwdiers <martin@…>

There was a lot of work on this, and then it all just stopped, waiting for Triage team to move to check-in. I would like to repeat permon's question above.

How do we give a "shout-out" to the triage members when a ticket has met check-in requirements, but yet has been sitting for months, particularly when we don't know who they are? (I understand it's a volunteer effort and all, but it's still a valid question).

I really need this patch, and it seems like many others do as well. As I am working on the newforms-admin branch, I would like to not have to apply this patch everytime I update SVN.

Changed 15 years ago by mwdiers <martin@…>

Restored tests to previously submitted patch

comment:33 Changed 15 years ago by Chris Beaven

Triage Stage: AcceptedReady for checkin

Yep, this does look ready for checkin to me. The best way to get tickets rolling again is to ask about them in django-dev group. Would you mind doing that now?

I'll promote anyway.

comment:34 in reply to:  33 Changed 15 years ago by mwdiers <martin@…>

Replying to SmileyChris:

Yep, this does look ready for checkin to me. The best way to get tickets rolling again is to ask about them in django-dev group. Would you mind doing that now?

Thank you. I will do so right away.

comment:35 Changed 15 years ago by wesley

Hi,
I have an issue with the latest patch (bigint-patch-2008-04-25.diff) and fixtures.
When I load a fixtures file that contains BigInteger fields, I have this error : "Value is too small/large to fit this field"

In file django/db/models/fields/init.py in class BigIntegerField in function get_db_prep_save(self, value).
The type of value parameter is unicode instead of long. (The content of value match correctly with the biginteger field in my fixtures file).

If I cast value parameter to long in the condition statement all works fine, here is my update :

    def get_db_prep_save(self, value):
        value_long =  long(value)
        if value_long > self.MAX_BIGINT or value_long < -self.MAX_BIGINT - 1: 
            raise ValueError("Value is to small/large to fit this field") 
        return super(BigIntegerField, self).get_db_prep_save(value)

comment:36 in reply to:  35 Changed 15 years ago by mwdiers <martin@…>

Patch needs improvement: set
Triage Stage: Ready for checkinUnreviewed

The value should have already been converted to a Long or an Integer by that point in the code. So the problem lies elsewhere. JSON and YAML serialization work just fine. It's only XML that is producing this error. From what I can tell, a regular IntegerField should also be producing this error, but for some reason it is not, and I cannot determine why not.

In any case, the proper solution for something like this is to override to_python():

    def to_python(self, value):
        return super(BigIntegerField, self).to_python(long(value))

I'm changing this bug to Unreviewed, as it's obviously not ready for checkin.

comment:37 Changed 15 years ago by mwdiers <martin@…>

Ok. I got it. The problem is introduced by trying to perform validation, and since IntegerField does no validation at all - by design - it produces no error here. Model is smart enough to parse out the integer by the time it gets to the DB.

But there is a bigger question: Why are we validating this field at all? Why not rely upon the backend to provide its own validation, just like we do with IntegerField?

Seems to me the fix is to skip validation entirely. That would also avoid the whole mess with different bigint sizes on different architectures.

comment:38 Changed 15 years ago by mwdiers <martin@…>

For anyone who would like BigInteger support, there is no need to wait for or to apply this patch. If you are using newforms, the following is all you need:

from django.db.models.fields import IntegerField
from django.conf import settings

class BigIntegerField(IntegerField):
	empty_strings_allowed=False
	def get_internal_type(self):
		return "BigIntegerField"
	
	def db_type(self):
		return 'NUMBER(19)' if settings.DATABASE_ENGINE == 'oracle' else 'bigint'

comment:39 Changed 15 years ago by Jacob

Triage Stage: UnreviewedAccepted

comment:40 in reply to:  37 Changed 15 years ago by Tomáš Kopeček

Replying to mwdiers <martin@diers.us>:

Ok. I got it. The problem is introduced by trying to perform validation, and since IntegerField does no validation at all - by design - it produces no error here. Model is smart enough to parse out the integer by the time it gets to the DB.

But there is a bigger question: Why are we validating this field at all? Why not rely upon the backend to provide its own validation, just like we do with IntegerField?

Seems to me the fix is to skip validation entirely. That would also avoid the whole mess with different bigint sizes on different architectures.

a) You're right, there is a possibility, that value arrives as string. So cast to long is necessary.
b) Why there is validation -> main reason was that each backend behaves differently. If I remember well, SQLite saves clipped value, MySQL raises exception and so on. For me looked like best way to have same behaviour with all backends.

Changed 15 years ago by Tomáš Kopeček

missing typecast

comment:41 Changed 15 years ago by Richard Davies <richard.davies@…>

Cc: richard.davies@… added

comment:42 Changed 15 years ago by Robert Coup

the current patch doesn't handle None values - get_db_prep_save() should explicitly check for it:

def get_db_prep_save(self, value):
    if value is not None:
        value_long =  long(value)
        if value_long > self.MAX_BIGINT or value_long < -self.MAX_BIGINT - 1: 
            raise ValueError("Value is to small/large to fit this field") 
    return super(BigIntegerField, self).get_db_prep_save(value)

comment:43 Changed 15 years ago by Martin Diers

I cannot speak for the developers, but when this patch has come up on the development group, time and again they have said that it is not needed, because it is now so easy to create your own special field types, on the fly, and in the case of a BigInt, it is trivial to do so. See the above example which I gave, dated 5/2/08.

I completely agree with this, especially considering the direction that Python 3 seems to be heading, where there is no longer any distinction at all between an Int and a Long, and the fact that 64-bit architectures are quickly relegating the old Int/Long distinction to an historical observation.

I would not be surprised if they eventually close this ticket as a "wontfix".

comment:44 in reply to:  43 Changed 15 years ago by Tomáš Kopeček

Triage Stage: AcceptedDesign decision needed

Replying to mwdiers:
There are two possibilities - we want to have BIGINT in all db backends, then this patch should be finished and incorporated into trunk. Second possibility is that bigint is not 'basic' field and we will discard this ticket. But it depends on somone from core developers. Again I think, that situation has changed after 1.0, so I will change status to 'design decision'.

Depending on result of this decision, I will try to finish the patch or discard it. From my point of view are both solutions valid.

comment:45 Changed 15 years ago by fnl

I created a fix for this problem which should work with Oracle and definitely works with PSQL and MySQL. It uses a new BigIntegerField, BigAutoField, and BigForeignKey field because they are all hardcoded to use the AUTO_INCREMENT feature of the databases. Would be nice if somebody could check out the code and comment if it works for Oracle, too.
link: http://www.djangosnippets.org/snippets/1244/

comment:46 Changed 15 years ago by CAJoel

I could not get mwdiers's patch to work or the fnl patch.

It appears to result in a KeyError on the next "manage.py reset SITE".

Traceback (most recent call last):

File "./manage.py", line 11, in <module>

execute_manager(settings)

File "/var/lib/python-support/python2.5/django/core/management.py", line 1672, in execute_manager

execute_from_command_line(action_mapping, argv)

File "/var/lib/python-support/python2.5/django/core/management.py", line 1630, in execute_from_command_line

output = action_mapping[action](mod, options.interactive)

File "/var/lib/python-support/python2.5/django/core/management.py", line 661, in reset

sql_list = get_sql_reset(app)

File "/var/lib/python-support/python2.5/django/core/management.py", line 352, in get_sql_reset

return get_sql_delete(app) + get_sql_all(app)

File "/var/lib/python-support/python2.5/django/core/management.py", line 468, in get_sql_all

return get_sql_create(app) + get_custom_sql(app) + get_sql_indexes(app)

File "/var/lib/python-support/python2.5/django/core/management.py", line 127, in get_sql_create

output, references = _get_sql_model_create(model, known_models)

File "/var/lib/python-support/python2.5/django/core/management.py", line 175, in _get_sql_model_create

col_type = data_types[data_type]

KeyError: 'BigIntegerField'

In the meantime, I've been using CharFields.

I wish this wasn't so hard to fix.

comment:47 Changed 15 years ago by CAJoel

The patches work in 1.0.2, but not in a pre 1.0 version which came with my distro.

I was hoping to avoid installing from source, bummer.

--joel

comment:48 Changed 15 years ago by Martin Diers

Joel,

I would never rely upon any distro to provide Django, unless you want to be stuck on a very old version of the framework, and ignore the incredible improvements which have come with 1.0 and 1.1. Installing from source is so trivial, it's not worth fretting over.

A massive amount has changed since the pre-1.0 days, including the ability to easily create custom field types, as I note in comment 38 above. 1.0 and later have no need for a patch like those in this ticket, and that is why no work further work is likely to be done to support BigInt in the back ends.

comment:49 Changed 15 years ago by nix

That advice is not necessarily true. I help maintain the django packages in the openSUSE devel:languages:python project available at:
http://download.opensuse.org/repositories/devel:/languages:/python/

We generally have the latest stable release (Currently at 1.0.2) of Django packaged and on the mirrors within 24 hours of release, and have worked together with the Django team in the past to do simultaneous releases of updated packages when security holes were found in Django. Please feel free to use our rpms if you want a tested Django installation on any currently shipping i386 or x86_64 openSUSE version and the most common SUSE Linux Enterprise versions (We don't auto-build IA64, Sparc, PowerPC or S390 packages at present, however people with these platforms can trivially rebuild our source rpms.)

As it turns out I also monitor this bug (and have submitted a patch to it in the past) as this bit me in one of my production deployments of Django.

comment:50 Changed 14 years ago by Walter Ferreira

Why this is not included yet in the django install source package?

Postgresql have Bigint field but django models not :|

comment:51 Changed 14 years ago by anonymous

@emonk:

Because anybody can implement Bigint by themselves with about 8 lines of code. See my comment above. It makes no sense to support new field types in Django that do not apply to all backends, when it is trivial to implement your own custom field with practically no work.

comment:52 Changed 14 years ago by fnl

@anonymous

Not true, see my comments on the possible implementation I submitted on djangosnippets that does not patch the django code but uses custom fields. Not only does it need 70 lines of (clean, readable) code, because to use bigint as PK you need to patch Integer, AutoField AND ForeignKey, otherwise it does not work properly. A very hiddeos and awful solution; but you are right if you want to just argue "it is practically no work" because you can copy my snippet...

comment:53 Changed 14 years ago by Gábor Farkas

Cc: gabor@… added

comment:54 Changed 14 years ago by Richard Davies <richard.davies@…>

milestone: 1.2

Adding to the 1.2 milestone, since it was accepted into the medium priority list (ORM-04).

Changed 14 years ago by Tomáš Kopeček

updated to trunk

comment:55 Changed 14 years ago by Tomáš Kopeček

Patch updated to trunk. Because oldforms library is completely out, some changes are out. Needs testing in admin/newforms environment. Also some better documentation (look for all relevant places in docs) is needed. Patch is against r11655.

comment:56 Changed 14 years ago by Tomáš Kopeček

Needs documentation: set
Needs tests: set

comment:57 Changed 14 years ago by jhovanny

Triage Stage: Design decision neededReady for checkin

Patch is not in trunk. I checked all changesets from r11655, and it's not there. Updated from trunk, and it's not there. Anybody knows what happened?

comment:58 in reply to:  57 Changed 14 years ago by Martin Diers

Triage Stage: Ready for checkinDesign decision needed

Replying to jhovanny:

Patch is not in trunk. I checked all changesets from r11655, and it's not there. Updated from trunk, and it's not there. Anybody knows what happened?

Nothing here says that the patch is in trunk. Permon's comment means he updated the patch so that it works against the trunk. In fact, if you look at the comments, you will see that the patch needs tests and better documentation. Please let the triage team change the triage stage.

comment:59 Changed 14 years ago by jhovanny

I apologize. I misunderstood the comment. I'll be testing this patch as part of my code, and report any problems that might arise.

comment:60 in reply to:  59 Changed 14 years ago by Martin Diers

Replying to jhovanny:

I apologize. I misunderstood the comment. I'll be testing this patch as part of my code, and report any problems that might arise.

"Needs Tests" refers to written Unit Tests. Please see the Contributing to Django page for more information about Triage, Unit Tests, and other useful topics.

comment:61 Changed 14 years ago by bruce_s

Is there any ETA on this? This bug has been open for four years. A working patch was submitted 08/26/05 -- four years ago. This still isn't in trunk.

Bigint is necessary to interoperate with Facebook (facebook id's are big integers), so there is substantial demand for this feature.

comment:62 Changed 14 years ago by Tomáš Kopeček

Needs documentation: unset
Needs tests: unset

Patch was little bit tailored to current trunk. Also tests were added. Tests were run on mysql and postgres backends. Please try other backends and let me know about potential problems. Documentation is according to documentation of other DB fields, so I am removing Doc flag.

comment:63 Changed 14 years ago by Ramiro Morales

bigint-patch-2009-11-23.diff contains an extraneous modification in django/contrib/comments/templates/comments/preview.html.

Changed 14 years ago by Tomáš Kopeček

updated patch, modelform, tests are back (removed typo in comments template)

comment:64 Changed 14 years ago by anonymous

I'm not sure if django/db/backends/sqlite3/creation.py shouldn't be patched this way:

+        'BigIntegerField':              'integer',

instead of actual:

+        'IntegerField':                 'bigint',

comment:65 in reply to:  64 ; Changed 14 years ago by Tomáš Kopeček

Triage Stage: Design decision neededAccepted

Replying to anonymous:
You are right. Patch was updated, also tests were slightly modified. Tests were successfully run on these backends: sqlite3, mysql, postgresql, postgresql_psycopg2. I am still looking for someone with running oracle instance who can confirm tests and correct admin behavior.
Also changing stage to accepted due Richard Davies

Changed 14 years ago by Tomáš Kopeček

updated patch (typo, tests)

comment:66 in reply to:  65 Changed 14 years ago by Chris Beaven

Replying to permon:

I am still looking for someone with running oracle instance who can confirm tests and correct admin behavior.

The best thing you can do is keep on bring it up on the django-dev google group.

comment:67 Changed 14 years ago by Karen Tracey

The tests from the previous patch ran on Oracle. I noticed the sqlite problem and some of the tests were failing on MySQL and Postgres, I assume your test tweaks involve no longer testing for the DB returning int, since in some cases the DB is going to be returning a long? (Sorry, trac isn't displaying the diff and I don't have much time right now.)

One thing that is not working, on Oracle, is introspection. Running inspecdb on a table with a field created as a BigInteger generates a model where the field is an Integer. This is something not covered by tests. It would be good if the tests included testing introspection. There are a few introspection tests in regressiontests/introspection but I have not had a chance to look at them at all to see how much guidance they give for testing stuff like this.

Changed 14 years ago by Ian Kelly

Added Oracle introspection and test

comment:68 Changed 14 years ago by Tomáš Kopeček

Patch needs improvement: unset

All tests works for me now, also introspection looks well now. Patch applies correctly to current trunk, so removing 'Patch needs improvement'. I'll consult committer/devel list about inclusion in trunk.

comment:69 Changed 14 years ago by Jiri Suchan

Patch needs improvement: set

Shouldn't be BigIntegerField compatible with IntegerField in all ways? Actually it isn't - if there's created BigIntegerField(blank=True, something), it throws TypeError during save with empty value in this field. Patch still needs improvements, imho.

long() argument must be a string or a number, not 'NoneType'

Exception Location: ../django_common/django/db/models/fields/init.py in get_db_prep_save, line 736

Changed 14 years ago by Tomáš Kopeček

Attachment: bigint-patch-20091215.diff added

fixed: correct saving null fields + test

comment:70 Changed 14 years ago by Jiri Suchan

Patch needs improvement: unset

I'm happy about it now. Taking back my needs_better_patch.

Changed 14 years ago by Karen Tracey

Attachment: kmt-bigint.diff added

comment:71 Changed 14 years ago by Karen Tracey

I've reviewed the existing patch and uploaded a new one.

Primary difference is that I removed the limit testing done during save. No other fields do this kind of validation during save and it seems highly inconsistent to add one field that has this one specific kind of test (without testing for other bad things, like attempting to save floats, for example). It is true that the behavior if you attempt to save outside the limits is ugly and inconsistent from db to db, but it's consistent with how other existing fields behave. The right way to fix it, consistently for all fields, would be in something like model validation, I think, which is beyond the scope of this ticket.

I did, however, leave in the formfield that sets max and min limits. This too is a bit inconsistent with other fields, but for some reason it does not bother me as much, perhaps because it limits the likelihood of a user encountering the ugliness resulting from not validating during save.

I also added some doc and put the existing doc for the new field in alphabetical order rather than making it a subtype under the doc for IntegerField. That's an implementation detail I don't believe needs to be exposed in the user doc.

comment:72 Changed 14 years ago by Karen Tracey

Resolution: fixed
Status: assignedclosed

(In [11887]) Fixed #399: Added big integer field. Thanks to Tomáš Kopeček for persistently maintaining a patch for this.

comment:73 Changed 12 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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