Opened 6 years ago

Closed 5 years ago

#26327 closed New feature (fixed)

Add JSONB_AGG to contrib.postgres

Reported by: Tomasz Nowak Owned by: Mads Jensen
Component: contrib.postgres Version: dev
Severity: Normal Keywords: JSON postgres aggregate
Cc: 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

PostgreSQL 9.3 added many features for the JSON data type.
They include JSON_AGG aggregate, which could join its friends (ArrayAgg and StringAgg) in django/contrib/postgres/aggregates/general.py, please find draft implementation.
Function description: http://www.postgresql.org/docs/9.3/static/functions-aggregate.html

Other (non-aggregate) functions and expressions could go to a new file, maybe django/contrib/postgres/expressions.py? Please find attached JSON_BUILD_OBJECT implementation. It takes an array of column names and converts it to alternating key/value pairs for the function; the key is an extracted column name. It works for me, but I guess the column name (the key) should be somehow sanitized before appending to sql_expressions.

Attachments (2)

contrib_postgres_json_agg.patch (903 bytes) - added by Tomasz Nowak 6 years ago.
expressions.py (1.5 KB) - added by Tomasz Nowak 6 years ago.

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by Tomasz Nowak

Changed 6 years ago by Tomasz Nowak

Attachment: expressions.py added

comment:1 Changed 6 years ago by Tim Graham

Needs documentation: set
Needs tests: set
Summary: JSON_AGG aggregate for PostgreSQLAdd JSON_AGG to contrib.postgres
Triage Stage: UnreviewedAccepted
Version: master

comment:2 Changed 6 years ago by Mads Jensen

I tried to add this, and also made a test that aggregates a single field; it's adapted from the StringAgg-function. Would it make more sense to augment it so it's easier to aggregate several fields, or is this something that's better done in Python?

Also, JSONB is only available in 9.4+, so maybe some version checking would be a good idea? It might be slightly misleading that the JSON-datatype in django.contrib.postgres.fields is actually jsonb.

    class JsonAgg(Aggregate):
        function = 'JSONB_AGG'
        template = '%(function)s(%(expressions)s)'
        _output_field = JSONField()
    
        def convert_value(self, value, expression, connection, context):
            if not value:
                return json.dumps({})
            return value

comment:3 in reply to:  2 Changed 6 years ago by Simon Charette

Would it make more sense to augment it so it's easier to aggregate several fields, or is this something that's better done in Python?

This can be done in a future iteration if required.

Also, JSONB is only available in 9.4+, so maybe some version checking would be a good idea?

You could throw an error by overriding as_sql() but I think a note in the documentation would be enough.

It might be slightly misleading that the JSON-datatype in django.contrib.postgres.fields is actually jsonb.

Agreed but lets not perpetuate this by naming the class JsonbAgg instead.

I find it a bit odd that we convert None to '' for StringConcat as it's not something we do for other aggregates (Count(NULL) is not converted to 0) and I'm not sure this is something we want to do for this aggregate as well. I can see a use case for having JsonbAgg return None and nothing prevents the users from wrapping the expression in a Coalesce if that's not what they expect.

comment:4 Changed 6 years ago by Simon Charette

Summary: Add JSON_AGG to contrib.postgresAdd JSONB_AGG to contrib.postgres

comment:5 Changed 6 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Patch needs improvement: set

PR with some test failures.

comment:6 Changed 6 years ago by Mads Jensen

Owner: set to Mads Jensen
Patch needs improvement: unset
Status: newassigned

comment:7 Changed 6 years ago by Tim Graham

Patch needs improvement: set

Please uncheck "Patch needs improvement" after addressing the PR comments.

comment:8 Changed 6 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:9 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 0a26f3c3:

Fixed #26327 -- Added JsonAgg to contrib.postgres.

Thanks Tim Graham for review.

comment:10 Changed 6 years ago by Tim Graham

Has patch: unset
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

As described in #27478, the current implementation doesn't work on PostgreSQL 9.4 and below.

comment:11 Changed 6 years ago by Mads Jensen

PR Updated changes to remediate test failure on PostgreSQL 9.4. There is a misleading naming convention with JSONField and JsonAgg. The manual highlights that Jsonb is the preferred option.

comment:12 Changed 6 years ago by Simon Charette

As mentioned above I think we should just rename the aggregate JSONBAgg (or whatever case variant we choose) and be done with it. It's unfortunate that we didn't name the contrib.postgres field JSONBField in the first place and have a database agnostic one named django.db.models.fields.JSONField now that other backends are shipping with support for such a field but we should make the distinction clear here.

We don't ship any field using the the PostgreSQL json datatype so it doesn't make much sense to ship an expression for the JSON_AGG SQL function IMHO. If we decide we still want to ship this tests should be added for it at least. That will require defining a JSONField relying on the json datatype.

comment:13 Changed 5 years ago by Tim Graham

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:14 Changed 5 years ago by Tim Graham <timograham@…>

In aa2cb4c:

Refs #26327 -- Renamed JsonAgg to JSONBAgg.

Thanks to Christian von Roques for the report.

comment:15 Changed 5 years ago by Tim Graham

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top