-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reimplement has_key
for SQLite
#6552
Comments
Here I explain the issue - I describe a bit more as I go through my thought process, that might be relevant for the implementation of the Old problemLet's consider this DB (I'm using SQLite Browser) Let's first understand how SELECT test_1.id, test_1.extras, anon_1.key, anon_1.value
FROM test AS test_1, json_each(JSON_QUOTE(JSON_EXTRACT(test_1.extras, '$'))) AS anon_1
WHERE json_type(test_1.extras, '$') = 'object' (here I'm just filtering on the first three rows that contain an object (i.e. a dict).
Therefore, the old query was simply filtering on these if there was a key with the correct value, something like SELECT test_1.id, test_1.extras, anon_1.key, anon_1.value
FROM test AS test_1, json_each(JSON_QUOTE(JSON_EXTRACT(test_1.extras, '$'))) AS anon_1
WHERE anon_1.key = 'b' The problem was the negation: now, the negation is applied to the whole filter! So this would be SELECT test_1.id, test_1.extras, anon_1.key, anon_1.value
FROM test AS test_1, json_each(JSON_QUOTE(JSON_EXTRACT(test_1.extras, '$'))) AS anon_1
WHERE anon_1.key != 'b' Indeed, this would return a result for every key-value pair where the key is not |
Implementing in AiiDA (SQLAlchemy)The implementation is now pretty straightforward - this seems to be doing the job: if operator == 'has_key':
from sqlalchemy import select
return select(database_entity).where(func.json_each(database_entity).table_valued('key', joins_implicitly=True).c.key == value).exists() How to test itHere is an extended test from the one of @mbercx in #6256: from aiida import orm, load_profile
from aiida.storage.sqlite_temp import SqliteTempBackend
temp_profile = SqliteTempBackend.create_profile('temp-profile')
load_profile(temp_profile, allow_switch=True)
node = orm.Int(1).store()
node.base.extras.set_many({'sub': {'a': 1, 'b': 2, 'c': 3}})
node = orm.Int(2).store()
node.base.extras.set_many({'sub': {'b': 2, 'c': 3}})
node = orm.Int(3).store()
node.base.extras.set_many({'sub': {'a': 1, 'b': 2}})
node = orm.Int(4).store()
node.base.extras.set_many({'sub': "a"})
node = orm.Int(5).store()
node.base.extras.set_many({'sub': 1})
node = orm.Int(6).store()
node.base.extras.set_many({'sub': ["a", "b"]})
node = orm.Int(7).store()
node.base.extras.set_many({'sub': ["c", "d"]})
qbuild = orm.QueryBuilder()
filters = {"extras.sub": {"!has_key": "a"}}
qbuild.append(
orm.Int,
project=['id', 'attributes.value'],
filters=filters,
)
print(qbuild.all())
print(qbuild.as_sql(True)) That prints
while replacing without negation:
This seems the correct result |
ConclusionThis provides the implementation that needs to be put in AiiDA. I didn't test it much more, and unfortunately I won't have time until the second half of August - so I'm putting this here in case someone who has time wants to implement it and add some tests (pinging @sphuber and @danielhollas as they were involved earlier, in addition to @mbercx) Also pinging @khsrali @eimrek @GeigerJ2 and @agoscinski as this is relevant for the support of Materials Cloud REST APIs directly via SQLite backends, and also for later benchmarking of performance. Note on testsI would add many tests, querying directly extras, some sub key of extras, in various forms, both dicts and lists, ints, floats, strings, bools, none - to make sure the queries work, and also that the result is identical between SQLite and PSQL |
Closed by #6606 |
In #6256, a bug was identified in the previous implementation of the
has_key
for SQLite. The bug is described there.To avoid returning wrong results, the feature has been disabled. We should re-enable it.
The text was updated successfully, but these errors were encountered: