Opened 3 years ago

Last modified 4 months ago

#33548 assigned Cleanup/optimization

Use -> operator to implement KeyTransform on SQLite 3.38+

Reported by: Sage Abdullah Owned by: Anže Pečar
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: sqlite jsonfield keytransform
Cc: bcail, rajdesai24, Paolo Melchiorre Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

SQLite added support for -> and ->> operators as of version 3.38.0. From the docs:

Both the -> and ->> operators select the same subcomponent of the JSON to their left. The difference is that -> always returns a JSON representation of that subcomponent and the ->> operator always returns an SQL representation of that subcomponent. Thus, these operators are subtly different from a two-argument json_extract() function call. A call to json_extract() with two arguments will return a JSON representation of the subcomponent if and only if the subcomponent is a JSON array or object, and will return an SQL representation of the subcomponent if the subcomponent is a JSON null, string, or numeric value.

It would be nice to have this as a cleanup/optimization, e.g. for the workaround introduced in this PR. Still, we need to check whether the SQLite bundled in the user's Python installation supports it though, so we can't just replace the current implementation.

Change History (14)

comment:1 by Mariusz Felisiak, 3 years ago

Summary: Use -> operator to implement KeyTransform on SQLite >= 3.38.0Use -> operator to implement KeyTransform on SQLite 3.38+
Triage Stage: UnreviewedAccepted

Thanks for the ticket. Tentatively accepted, it can be tricky to implement.

comment:2 by bcail, 2 years ago

Cc: bcail added

For what it's worth, the SQLite tests pass on SQLite 3.39.4 if you replace JSON_EXTRACT with ->>, like this:

diff --git a/django/db/models/fields/json.py b/django/db/models/fields/json.py
index 7296fe42bc..accc6ae61a 100644
--- a/django/db/models/fields/json.py
+++ b/django/db/models/fields/json.py
@@ -361,7 +361,7 @@ class KeyTransform(Transform):
         )
         return (
             "(CASE WHEN JSON_TYPE(%s, %%s) IN (%s) "
-            "THEN JSON_TYPE(%s, %%s) ELSE JSON_EXTRACT(%s, %%s) END)"
+            "THEN JSON_TYPE(%s, %%s) ELSE (%s ->> %%s) END)"
         ) % (lhs, datatype_values, lhs, lhs), (tuple(params) + (json_path,)) * 3
 

comment:3 by rajdesai24, 22 months ago

Owner: changed from nobody to rajdesai24
Status: newassigned

comment:4 by rajdesai24, 21 months ago

I am quite new to this topic but based on what I can understand how about we put a simple version check and return the value accordingly
something like the following which checks the version and returns the value accordingly.

        if connection.sqlite_version >= '3.38':
                    return (
                    "(CASE WHEN JSON_TYPE(%s, %%s) IN (%s) "
                    "THEN JSON_TYPE(%s, %%s) ELSE (%s ->> %%s) END)"
                    ) % (lhs, datatype_values, lhs, lhs), (tuple(params) + (json_path,)) * 3
 
        else:
            return (
            "(CASE WHEN JSON_TYPE(%s, %%s) IN (%s) "
            "THEN JSON_TYPE(%s, %%s) ELSE JSON_EXTRACT(%s, %%s) END)"
            ) % (lhs, datatype_values, lhs, lhs), (tuple(params) + (json_path,)) * 3


This might make it redundant but please let me know of any suggestions. Till then I will dive deeper

comment:5 by rajdesai24, 21 months ago

Cc: rajdesai24 added

comment:6 by Paolo Melchiorre, 14 months ago

Cc: Paolo Melchiorre added

This change would be an improvement. +1

comment:7 by bcail, 8 months ago

@rajdesai24, are you planning to open a PR for this?

comment:8 by Claude Paroz, 8 months ago

Owner: rajdesai24 removed
Status: assignednew

13 months after an assignee last commented, I guess we can assume it is open to anyone willing to work on it.

comment:9 by Anže Pečar, 5 months ago

Owner: set to Anže Pečar
Status: newassigned

I have found this ticket during DjangoCon Europe sprints and opened a PR for it: https://github.com/django/django/pull/18241

I didn't add the version check discussed about because Python 3.10.9 updated SQLite to 3.39 so the operator should be supported on all Python versions that Django currently supports.

Version 1, edited 5 months ago by Anže Pečar (previous) (next) (diff)

comment:10 by Paolo Melchiorre, 5 months ago

As we discussed during the DjangoCon Europe 2024 sprints, we can't be sure of the SQLite version in some cases, so a version check can be useful.

comment:11 by Anže Pečar, 5 months ago

I have added the version check to my PR, thank you Paolo, for the quick PR review!

comment:12 by Paolo Melchiorre, 5 months ago

The PR looks better now. Thanks for update it.

comment:13 by Jacob Walls, 5 months ago

Has patch: set

comment:14 by Mariusz Felisiak, 4 months ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top