[Django] #26327: JSON_AGG aggregate for PostgreSQL
Django
----------------------------------+-------------------------------------
Reporter: tomaszn | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version:
Severity: Normal | Keywords: JSON postgres aggregate
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26327>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tomaszn | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version:
Keywords: JSON postgres aggregate | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 0
UI/UX: 0 |
Changes (by tomaszn):
* Attachment "contrib_postgres_json_agg.patch" added.
Django
Reporter: tomaszn | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version:
Keywords: JSON postgres aggregate | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 0
UI/UX: 0 |
Changes (by tomaszn):
* Attachment "expressions.py" added.
Django
-------------------------------------+-------------------------------------
Reporter: tomaszn | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 1
* version: => master
* needs_docs: => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:1>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Comment (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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: (none)
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: (none)
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: (none)
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
* needs_docs: 1 => 0
Comment:
[https://github.com/django/django/pull/7335 PR] with some test failures.
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:5>
Django
-------------------------------------+-------------------------------------
| Jensen
Type: New feature | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mads Jensen):
* owner: (none) => Mads Jensen
* needs_better_patch: 1 => 0
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Type: New feature | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
Comment:
Please uncheck "Patch needs improvement" after addressing the PR comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Type: New feature | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Component: contrib.postgres | Version: master
Severity: Normal | Resolution: fixed
Keywords: JSON postgres | Triage Stage: Ready for
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0a26f3c3388137c336fb1417ba870fde12c0fbcc" 0a26f3c3]:
{{{
#!CommitTicketReference repository=""
revision="0a26f3c3388137c336fb1417ba870fde12c0fbcc"
Fixed #26327 -- Added JsonAgg to contrib.postgres.
Thanks Tim Graham for review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham):
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
As described in #27478, the current implementation doesn't work on
PostgreSQL 9.4 and below.
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Mads Jensen):
[https://github.com/django/django/pull/7554 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Severity: Normal | Resolution:
aggregate | checkin
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Severity: Normal | Resolution:
Keywords: JSON postgres | Triage Stage: Ready for
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"aa2cb4c622dfd5709ba89ac3cace959202318987" aa2cb4c]:
{{{
#!CommitTicketReference repository=""
revision="aa2cb4c622dfd5709ba89ac3cace959202318987"
Refs #26327 -- Renamed JsonAgg to JSONBAgg.
Thanks to Christian von Roques for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Tomasz Nowak | Owner: Mads
| Jensen
Component: contrib.postgres | Version: master
Severity: Normal | Resolution: fixed
Keywords: JSON postgres | Triage Stage: Ready for
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/26327#comment:15>