[Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
#29482: simplify KeyTransform to always use the 'nested_operator'
------------------------------------------+------------------------
               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.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/048.d299a184c6e1ab0f832194c22bd1ee23%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
#29482: simplify KeyTransform to always use the 'nested_operator'
--------------------------------+--------------------------------------
     Reporter:  David De Sousa  |                    Owner:  nobody
         Type:  Uncategorized   |                   Status:  new
    Component:  Uncategorized   |                  Version:  2.0
     Severity:  Normal          |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.ab1d7659e0c1c141d6884886f7c2b9d8%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
In reply to this post by Django
#29482: simplify KeyTransform to always use the 'nested_operator'
--------------------------------+--------------------------------------
     Reporter:  David De Sousa  |                    Owner:  nobody
         Type:  Uncategorized   |                   Status:  new
    Component:  Uncategorized   |                  Version:  2.0
     Severity:  Normal          |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.7ec498bbc11897a00afda13d486abe34%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
In reply to this post by Django
#29482: simplify KeyTransform to always use the 'nested_operator'
--------------------------------+--------------------------------------
     Reporter:  David De Sousa  |                    Owner:  nobody
         Type:  Uncategorized   |                   Status:  new
    Component:  Uncategorized   |                  Version:  2.0
     Severity:  Normal          |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.92daeb6b92ec7704a9f4792c84b94be7%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
In reply to this post by Django
#29482: simplify KeyTransform to always use the 'nested_operator'
--------------------------------+--------------------------------------
     Reporter:  David De Sousa  |                    Owner:  nobody
         Type:  Uncategorized   |                   Status:  new
    Component:  Uncategorized   |                  Version:  2.0
     Severity:  Normal          |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.9a1eac8b1901851597115ead5053bcc1%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
In reply to this post by Django
#29482: simplify KeyTransform to always use the 'nested_operator'
--------------------------------+--------------------------------------
     Reporter:  David De Sousa  |                    Owner:  nobody
         Type:  Uncategorized   |                   Status:  new
    Component:  Uncategorized   |                  Version:  2.0
     Severity:  Normal          |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.cb7578ca7fdb2887a276d0b363b81e8f%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
In reply to this post by Django
#29482: simplify KeyTransform to always use the 'nested_operator'
--------------------------------+--------------------------------------
     Reporter:  David De Sousa  |                    Owner:  nobody
         Type:  Uncategorized   |                   Status:  new
    Component:  Uncategorized   |                  Version:  2.0
     Severity:  Normal          |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.cf8403bd7005dbc7c5f80eeb5a04c0a2%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29482: simplify KeyTransform to always use the 'nested_operator'

Django
In reply to this post by Django
#29482: simplify KeyTransform to always use the 'nested_operator'
--------------------------------+--------------------------------------
     Reporter:  David De Sousa  |                    Owner:  nobody
         Type:  Uncategorized   |                   Status:  new
    Component:  Uncategorized   |                  Version:  2.0
     Severity:  Normal          |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.49ead3c392e7bf5d7cb2fd34da56d431%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.