Fixed #33548 -- Use ->> operator to implement KeyTransform on SQLite 3.38+ #18241
Conversation
|
Can we add a test for this? |
I think there are plenty of tests already for this in the suite, the question is more along the lines of whether CI is running SQLite < or >= 3.38+ and exercise this new code path. One way to test it out locally is via |
|
I'm not sure how to check what Jenkins is running, but Debian bookworm is on 3.40 while Debian bullseye is on 3.36. I'm happy to add a test that mocks the |
|
Hmm, if we don't cover this only because Jenkins doesn't have both versions, I think it's fine as-is if local testing passes. |
| "(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 | ||
| if connection.get_database_version() >= (3, 38): |
There was a problem hiding this comment.
I'd change it to the SQLite-specific feature flag.
There was a problem hiding this comment.
I'll add the feature flag if we decide to move forward with this (#18241 (comment))
| 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 |
There was a problem hiding this comment.
What about comment in SQLite operations:
django/django/db/backends/sqlite3/operations.py
Lines 27 to 29 in cf03aa4
Do we still need a JSON_TYPE workaround? If yes, I don't see much value in this change, TBH 🤷
There was a problem hiding this comment.
From what I can tell the ->> operator behaves the same way as JSON_EXTRACT so we still need the JSON_TYPE workaround if we use it.
The -> operator, handles the null, false, true values the way as the CASE statement with JSON_TYPE:
sqlite> select JSON_TYPE('null', '$');
null
sqlite> select JSON_TYPE('true', '$');
true
sqlite> select JSON_TYPE('false', '$');
false
sqlite> select 'null' -> "$";
null
sqlite> select 'true' -> "$";
true
sqlite> select 'false' -> "$";
false
But there are other differences, one example:
sqlite> select JSON_EXTRACT('"7"', "$");
7
sqlite> select '"7"' -> "$";
"7"
If I switch the CASE statement with -> it causes failures=29, errors=2 in test_jsonfield.py, and I'm not exactly sure how to fix it.
I think you might be correct about there not being much value in having this merged. I'm happy if we can close the ticket as won't do instead.
|
I can check it next week with various SQLite versions. |
@sarahboyce I have not, but I can update those if we decide to go forward with this and how (ref: #18241 (comment)). |
Trac ticket number
ticket-33548
Branch description
Use the ->> operator to implement KeyTransform for JSON fields on SQLite.
I didn't add the version check because Python 3.10 upgraded SQLite to 3.39 in the 3.10.9 release https://docs.python.org/3.10/whatsnew/changelog.html#id26
Checklist
mainbranch.