Opened 14 years ago

Last modified 12 years ago

#13844 closed Bug

Errors when using character fields for aggregation — at Version 6

Reported by: zegrep@… Owned by: Greg Wogan-Browne
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: philipe.rp@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Julien Phalip)

My intension is joining tables with two different formated columns,
by using django's double underscore magic to resolve the querysets.

The values in table B are preceded by leading zeros.

table A --> id = "10"

table B --> id = "000010"

select distinct b.id from a, b where where a.id = TRIM(leading '0' FROM b.id);

The resulting code should look like this

    qs_a=A.objects.all().values('id')
    qs_b=B.objects.annotate(id_trim=Trim('id', position='leading', char='0')).filter(id_trim__in=qs_a)

I use the following code to implement the extra functionality.

try:
    import psycopg2
except ImportError, e:
    import psycopg
else:
    psycopg = psycopg2._psycopg

class Trim(django.db.models.Aggregate):
    name = 'Trim_pg'
django.db.models.Trim=Trim

class Trim_pg(django.db.models.sql.aggregates.Aggregate):
    '''
        position = [leading, trailing, both]
        char = <character to remove>
    '''

    sql_function = 'TRIM'
    sql_template = '''%(function)s(%(position)s %(char)s FROM %(field)s)'''

    def __init__(self, col, distinct=False, **extra):
        assert extra.has_key('position'), u'no position'
        assert extra['position'] in ('leading', 'trailing', 'both'), 'position no in [leading, trailing, both]'
        assert extra.has_key('char'), u'no char'
        assert len(extra['char']) == 1, 'only one character'
        extra['char']=str(psycopg2._psycopg.QuotedString(extra['char'])) #Quoting
        super(Trim_pg, self).__init__(col, distinct=distinct, **extra)
django.db.models.sql.aggregates.Trim_pg=Trim_pg

The problem is, that "convert_values" makes for a "CharField"
a cast to float. My solution is to return the value for CharFields
without the cast.

Index: db/backends/__init__.py
===================================================================
--- db/backends/__init__.py (Revision 12595)
+++ db/backends/__init__.py (Arbeitskopie)
@@ -438,6 +438,8 @@
             return int(value)
         elif internal_type in ('DateField', 'DateTimeField', 'TimeField'):
             return value
+        elif internal_type in  ('CharField'):
+            return value
         # No field, or the field isn't known to be a decimal or integer
         # Default to a float
         return float(value)

Change History (6)

comment:1 by Chris Beaven, 14 years ago

Component: UncategorizedDatabase layer (models, ORM)
Summary: ValueError: by adding custom aggregate function TRIMErrors when using character fields for aggregation
Triage Stage: UnreviewedAccepted
Version: 1.11.2

For the life of me I can't understand the logic of the original convert_values method, but it definitely seems wrong.

Someone in IRC encountered a problem because of this using the built in Max aggregation class with a charfield, so it's not just custom aggregate classes which are effected here.

comment:2 by Felipe 'chronos' Prenholato, 14 years ago

Cc: philipe.rp@… added
Has patch: set

I'm the guy at IRC. The proposed fix already works for me. I having troubles in get Max from a charfield.

Given model:

class Link_A(models.Model):
    link = models.CharField(max_length=500)
    short_link = models.CharField(max_length=50)

I get following error when try to get Max of short_link:

In [5]: Link_A.objects.all().aggregate(Max('short_link'))
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)

/home/felipe/projects/testproject/<ipython console> in <module>()

/home/felipe/.py/2.6/lib/python2.6/site-packages/django/db/models/query.pyc in aggregate(self, *args, **kwargs)
    311                 is_summary=True)
    312 
--> 313         return query.get_aggregation(using=self.db)
    314 
    315     def count(self):

/home/felipe/.py/2.6/lib/python2.6/site-packages/django/db/models/sql/query.pyc in get_aggregation(self, using)
    371             (alias, self.resolve_aggregate(val, aggregate, connection=connections[using]))
    372             for (alias, aggregate), val
--> 373             in zip(query.aggregate_select.items(), result)
    374         ])
    375 

/home/felipe/.py/2.6/lib/python2.6/site-packages/django/db/models/sql/query.pyc in resolve_aggregate(self, value, aggregate, connection)
    325         else:
    326             # Return value depends on the type of the field being processed.

--> 327             return self.convert_values(value, aggregate.field, connection)
    328 
    329     def get_aggregation(self, using):

/home/felipe/.py/2.6/lib/python2.6/site-packages/django/db/models/sql/query.pyc in convert_values(self, value, field, connection)
    303         it can be overridden by Query classes for specific backends.
    304         """
--> 305         return connection.ops.convert_values(value, field)
    306 
    307     def resolve_aggregate(self, value, aggregate, connection):

/home/felipe/.py/2.6/lib/python2.6/site-packages/django/db/backends/__init__.pyc in convert_values(self, value, field)
    443         # No field, or the field isn't known to be a decimal or integer

    444         # Default to a float

--> 445         return float(value)
    446 
    447     def check_aggregate_support(self, aggregate_func):

ValueError: invalid literal for float(): c

and checking queries, I can see that querie are made:

In [6]: connection.queries
Out[6]: 
[{'sql': 'SELECT MAX("webui_link_a"."short_link") AS "short_link__max" FROM "webui_link_a"',
  'time': '0.001'}]

That's is a small fix, but I think that need to check how act with others internal types, or default to float only if float(value) don't raise nothing and just return value if raise.

comment:3 by Matthias Kestenholz, 14 years ago

This is a related issue: #12889 Using annotation unexpectedly returns DecimalFields as floats

comment:4 by Greg Wogan-Browne, 14 years ago

Owner: changed from nobody to Greg Wogan-Browne
Status: newassigned

comment:5 by Graham King, 13 years ago

Severity: Normal
Type: Bug

comment:6 by Julien Phalip, 13 years ago

Description: modified (diff)
Needs tests: set
Patch needs improvement: set

Patch needs improvement as per chronos' comment. Also needs tests.

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