Skip to content
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

Use only 1 retrieval query for similar requests #191

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

CasperWA
Copy link
Collaborator

As it stands 4 QueryBuilder queries are performed during the absolute first query of a server's lifetime.
After that only 3 queries are performed.

If a filter is included and it targets a calculated OPTIMADE field stored in the Nodes' extras, another query will be added (5 then 4).

This PR drops the queries performed per request substantially to a minimum of 1 query per request.

This is done by manually subtracting offset from the counted results of the query when determining whether there are more_data_available.

It also now stores the latest count and checks whether an option is supplied to the query that would alter it (filters, limit, offset). If any of these have changed since the last time count was called it will call count again.
This also takes into account if there is a change, but the default value is used. E.g., if page_offset was not used in the original request (storing a None value in the server), but then it's set to 0 in a subsequent request (without altering anything else about the request). This will not change the result of counting, since an offset of value 0 is the default, hence equivalent to None.

Finally, the checked extras fields are now stored. So if a filter is given which filters on an OPTIMADE field that is calculated and stored in the Nodes' extras, it will be checked (resulting in a QueryBuilder query) and stored in a set. All subsequent requests that filters on the same OPTIMADE fields will not result in a new QueryBuilder query to check whether the fields are present in all Nodes' extras (since this has already now been checked and confirmed).
Note: This also means that one should restart a server after changing the served database if, e.g., new Nodes are added.

@CasperWA CasperWA added the enhancement New feature or request label Jan 14, 2021
@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #191 (2b97e37) into develop (88095d3) will increase coverage by 0.40%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #191      +/-   ##
===========================================
+ Coverage    90.51%   90.92%   +0.40%     
===========================================
  Files           30       30              
  Lines         1223     1267      +44     
===========================================
+ Hits          1107     1152      +45     
+ Misses         116      115       -1     
Flag Coverage Δ
pytest 90.92% <100.00%> (+0.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida_optimade/entry_collections.py 95.33% <100.00%> (+1.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88095d3...2b97e37. Read the comment docs.

Add tests - update accordingly:
No 'limit' special condition in `count()` method.
`None` is not the same as the default `page_limit` value, since the
underlying `_find()` method doesn't use the default `page_limit`, but
rather simply sets limit to `None`.

Testing standard page_offset as well as standard use of page_limit and
also whether the max page_limit set in the configuration file is
respected.

Check how many times QueryBuilder is reported to be called in different
scenarios.

Also, add test similar to `test_count_offset` and `test_count_limit` to
test filters during count.
@CasperWA CasperWA merged commit 2b97e37 into aiidateam:develop Jan 17, 2021
@CasperWA CasperWA deleted the minimize_qb_calls branch January 17, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant