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

Handle arbitrary nested NOT/AND/OR in queries #221

Merged
merged 3 commits into from Mar 13, 2020
Merged

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Mar 10, 2020

This PR allows us to recursively parse nested queries, but does NOT fix the problem described in #79 (yet...). Essentially the expression_phrase function fails to go deep enough through the tree and ends up flattening out the query at the first AND/OR, which breaks any negations.

An example of the tests failing against master can be found here, which amounts to NOT (expr1 AND expr2) becoming NOT expr1 and expr2 instead of NOT expr1 AND NOT expr2. This PR fixes that by extending the negation to the second expression in this example, and hopefully to arbitrary expressions.

The discussion in #79 amounts to the distinction between the above and NOT (expr1 AND expr2) being tricky to handle in Mongo as it stands, which instead would need something like (expr1 AND NOT expr2) OR (NOT expr1 AND expr2), which should probably be handle with care in another PR.

@ml-evs ml-evs added bug Something isn't working transformer/MongoDB Related to filter transformer for MongoDB labels Mar 10, 2020
@ml-evs ml-evs added this to In progress in Road to optimade-python-tools 1.0 via automation Mar 10, 2020
@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #221 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   87.55%   87.57%   +0.02%     
==========================================
  Files          43       43              
  Lines        1912     1916       +4     
==========================================
+ Hits         1674     1678       +4     
  Misses        238      238
Flag Coverage Δ
#unittests 87.57% <100%> (+0.02%) ⬆️
Impacted Files Coverage Δ
optimade/filtertransformers/mongo.py 95.65% <100%> (+0.19%) ⬆️

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 2ed7f74...ee1a400. Read the comment docs.

@ml-evs ml-evs marked this pull request as ready for review March 10, 2020 20:11
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Seems legit. Thanks.
You can leave out my comment changes if you want - especially the longer one. I just thought to make the negation more clear in the comment, but I see now that then the nor comment should change as well...

Furthermore, a couple of questions.

optimade/filtertransformers/mongo.py Show resolved Hide resolved
optimade/filtertransformers/mongo.py Outdated Show resolved Hide resolved
optimade/filtertransformers/mongo.py Outdated Show resolved Hide resolved
optimade/filtertransformers/mongo.py Outdated Show resolved Hide resolved
tests/filtertransformers/test_mongo.py Show resolved Hide resolved
ml-evs and others added 3 commits March 13, 2020 14:25
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Seems good to me 👍

It's a shame about the quirks with Lark not permitting recursive methods on the ones it called. One could consider making the recursive method a function in the expression_phrase method ... but in the end its all the same, and this, honestly, looks prettier than my suggestion.

@ml-evs
Copy link
Member Author

ml-evs commented Mar 13, 2020

Seems good to me +1

It's a shame about the quirks with Lark not permitting recursive methods on the ones it called. One could consider making the recursive method a function in the expression_phrase method ... but in the end its all the same, and this, honestly, looks prettier than my suggestion.

Yeah, I considered having some kind of closure inside expression_phrase but I'd be more worried about unintended side effects if we did it that way.

@ml-evs ml-evs merged commit 5569bda into master Mar 13, 2020
Road to optimade-python-tools 1.0 automation moved this from In progress to Done Mar 13, 2020
@ml-evs ml-evs deleted the ml-evs/not_and_queries branch March 13, 2020 15:16
@ml-evs ml-evs mentioned this pull request Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working transformer/MongoDB Related to filter transformer for MongoDB
Development

Successfully merging this pull request may close these issues.

None yet

2 participants