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

Fix cases where comparison first and property first queries did not match #1134

Merged
merged 3 commits into from Apr 30, 2022

Conversation

JPBergsma
Copy link
Contributor

Rerouted 'constant_first_comparison' via 'property_first_comparison' in optimade/filtertransformers/mongo.py so $ne and $size are handled properly for constant_first_comparisons.

Closes #1133

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #1134 (464f0b9) into master (0fb4423) will increase coverage by 0.03%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #1134      +/-   ##
==========================================
+ Coverage   92.33%   92.37%   +0.03%     
==========================================
  Files          68       68              
  Lines        3849     3881      +32     
==========================================
+ Hits         3554     3585      +31     
- Misses        295      296       +1     
Flag Coverage Δ
project 92.37% <94.44%> (+0.03%) ⬆️
validator 92.37% <94.44%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
optimade/server/warnings.py 88.88% <50.00%> (+0.65%) ⬆️
optimade/server/query_params.py 98.03% <96.66%> (-1.97%) ⬇️
optimade/filtertransformers/mongo.py 96.66% <100.00%> (ø)
optimade/server/config.py 91.39% <100.00%> (+0.09%) ⬆️
optimade/server/routers/utils.py 98.13% <100.00%> (+0.03%) ⬆️

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 1408a7c...464f0b9. Read the comment docs.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test that fails without these proposed changes, and passes with them? I imagine you have implicitly done this by adding the extra reference test case, but it would be good to see it clearly.

@JPBergsma
Copy link
Contributor Author

JPBergsma commented Apr 29, 2022

I have added a few test for the MongoDB filter transformer.
The extra reference test case will indeed also trigger this bug.

@ml-evs ml-evs added bug Something isn't working transformer/MongoDB Related to filter transformer for MongoDB server Issues pertaining to the example server implementation labels Apr 30, 2022
@ml-evs ml-evs changed the title Rerouted 'constant_first_comparison' via 'property_first_comparison' to fix issue #1133 Fix cases where comparison first and property first queries did not match Apr 30, 2022
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

…in optimade/filtertransformers/mongo.py so and are handled priperly for constant_first_comparisons.
Added test for handling != in constant first comparissons.
@ml-evs ml-evs enabled auto-merge (squash) April 30, 2022 11:50
@ml-evs ml-evs merged commit 377a53d into Materials-Consortia:master Apr 30, 2022
@JPBergsma JPBergsma deleted the JPBergsma_fix_1133 branch April 30, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Issues pertaining to the example server implementation transformer/MongoDB Related to filter transformer for MongoDB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queries with the form: 'value != prop' return entries where 'prop == None'
2 participants