[Django] #29482: simplify KeyTransform to always use the 'nested_operator'
Django
------------------------------------------+------------------------
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
Currently, the `KeyTransform` and `KeyTextTransform` classes in the JSONB
Field lookups are making a distinction in the way the query is being
generated based on the depth of the key inside the JSON, using a different
operator depending on if the key is in the root of the json or not.
This is an issue specially if you create indexes based on jsonb
traversals, for some reason if you create an index with the path operators
(#> or #>>) and then query with the "regular" operators (-> or ->>) the
indexes are not always used.
I've tested removing the use of the `operator` and basically just leaving
the return in
https://github.com/django/django/blob/2.0.6/django/contrib/postgres/fields/jsonb.py#L105
as the only return in the function and it works for me, I'd be happy to
create a PR but I don't know if that distinction is there for a reason.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
From my understanding the `->` and `->>` operators are used instead of
their `#` variant when the key lookup depth is 1 for readability purposes.
Is there a reason you can't create your non-nested attribute lookups using
the `->` and `->>` operators instead? Are you suggesting we favor the `#`
syntax because a functional index on `data#>'{a,b}'` would be used when
doing a `data#>'{a}'` lookup?
I'm asking because changing it at this point is likely to break any
previously created functional index created using these operators for the
same reason you are asking them to be changed here. I assume this is
something that would eventually be fixed in PostgreSQL.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:1>
Django
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
Does it need something to do with array elements vs object keys? Or does
the postgres nested operator already handle that?
If you had a top-level (and non-nested) JSON array in a field, you'd
probably want to stick to a simpler index/operator, I think.
I haven't looked into this, it's just thoughts.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:2>
Django
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David De Sousa):
Replying to [comment:1 Simon Charette]:
> From my understanding the `->` and `->>` operators are used instead of
their `#` variant when the key lookup depth is 1 for readability purposes.
>
> Is there a reason you can't create your non-nested attribute lookups
using the `->` and `->>` operators instead? Are you suggesting we favor
the `#` syntax because a functional index on `data#>'{a,b}'` would be used
when doing a `data#>'{a}'` lookup?
No, that won't happen AFAIK
>
> I'm asking because changing it at this point is likely to break any
previously created functional index created using these operators for the
same reason you are asking them to be changed here. I assume this is
something that would eventually be fixed in PostgreSQL.
It would be awesome if PostgreSQL dealt with this, but I asked in the
#postgres IRC and was told there's no way for the query planner to do
this, although I got confirmation the `#` and `-` operators are
equivalent.
No, the issue is that django uses both operators based on if it's a root
key or a nested one, I agree a change here would potentially break
previously created indexes using the `->` or `-->` operators, so
developers should take this into consideration when building indexes, it
should at least be documented, I found out the hard way.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:3>
Django
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David De Sousa):
Replying to [comment:2 Matthew Schinckel]:
> Does it need something to do with array elements vs object keys? Or does
the postgres nested operator already handle that?
`#>>` is a path operator, so it will work also with array indexes, #>>
'{2}' would give you the 3rd element of the root array.
>
> If you had a top-level (and non-nested) JSON array in a field, you'd
probably want to stick to a simpler index/operator, I think.
As I said before I could agree with this, although I'd vouch for
simplicity in the code, using only the path operator makes the code a lot
simpler IMHO
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:4>
Django
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by James Howe):
The suggested change doesn't solve the problem, it just creates a new one.
The problem is that an index will only be used if the JSON operators are
the same in both the index definition and the query.
The four path traversal operators (`->`, `->>`, `#>`, `#>>`) are all
incompatible.
Standardizing on a single operator for all queries (I suggest `->`) would
make things easier to deal with, but people may still have indices they
are unable to use.
Allowing the operator to be controlled may be a quick workaround.
Ideally, the `Index` classes should support JSON paths (and partial
indexes), so the definition can be standardized to the same operators.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:5>
Django
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by James Howe):
* cc: James Howe (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:6>
Django
Reporter: David De Sousa | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David De Sousa):
I agree, the ideal solution would be to support JSON paths and partial
indexes in the `Index` class, just be aware that -> and ->> return
different types, so instead of using the four operators, django could use
two, either -> and ->> or #> and #>>, depending on the user's desire.
Although if support is added to the `Index` class, this discussion is
irrelevant since the index can be created with the same logic the
`KeyTransform` classes use, using the -> and ->> operators when dealing
with root-level keys and the #> and #>> operators when traversing, this is
what I'm doing in a management command that handles the index creation for
my use case after I found out about this behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:7>
Django
Reporter: David De Sousa | Owner: nobody
Component: contrib.postgres | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
* component: Uncategorized => contrib.postgres
* stage: Unreviewed => Accepted
Comment:
I'm not sure what the way forward is, but it sounds like Django can do
something, whether it's a behavior change or some documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:8>
Django
Reporter: David De Sousa | Owner: nobody
Cleanup/optimization |
Component: contrib.postgres | Version: 2.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => wontfix
* stage: Accepted => Unreviewed
Comment:
Replying to [comment:7 David De Sousa]:
> Although if support is added to the `Index` class, this discussion is
irrelevant since the index can be created with the same logic the
`KeyTransform` classes use, using the -> and ->> operators when dealing
with root-level keys and the #> and #>> operators when traversing, this is
what I'm doing in a management command that handles the index creation for
my use case after I found out about this behavior.
`Index()` supports creating functional and partial indexes based on
`JSONField` key transforms (see
[https://github.com/django/django/blob/f97401d1b184406d2e24f11eddbdaca8bbc360e3/tests/schema/tests.py#L3108-L3155
tests]), so as far as I'm aware this can be closed.
Closing as "wontfix", because we will not change the current operators.
--
Ticket URL: <https://code.djangoproject.com/ticket/29482#comment:9>