-
Notifications
You must be signed in to change notification settings - Fork 44
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
557426a
to
5177ed5
Compare
Co-authored-by: Casper Welzel Andersen <[email protected]>
5177ed5
to
ee1a400
Compare
There was a problem hiding this 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.
Yeah, I considered having some kind of closure inside |
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)
becomingNOT expr1 and expr2
instead ofNOT 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.