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

Queries with the form: 'value != prop' return entries where 'prop == None' #1133

Closed
JPBergsma opened this issue Apr 28, 2022 · 7 comments · Fixed by #1134
Closed

Queries with the form: 'value != prop' return entries where 'prop == None' #1133

JPBergsma opened this issue Apr 28, 2022 · 7 comments · Fixed by #1134
Labels
bug Something isn't working transformer/MongoDB Related to filter transformer for MongoDB

Comments

@JPBergsma
Copy link
Contributor

For the trajectory endpoint, I added a few references without DOI values.
A query like https://optimade-trajectories.herokuapp.com/v1/references?filter=%2210.1038/00000%22!=doi returned 4 entries, while the query
https://optimade-trajectories.herokuapp.com/v1/references?filter=doi!=%2210.1038/00000%22 returns 2 entries

This also applies to other reference properties like journal and year.

@ml-evs
Copy link
Member

ml-evs commented Apr 28, 2022

Hmm, I remember adding this as an explicit edge case with MongoDB here:

# Awkwardly, MongoDB will match null fields in $ne filters,
# so we need to add a check for null equality in evey $ne query.
if "$ne" in query:
return {"$and": [{quantity: query}, {quantity: {"$ne": None}}]}

I guess it just needs to be percolated down to the "constant first" expression style. Perhaps you could add some tests outside of the trajectories PR that tests some reversed queries like this? I think we have a few already but maybe != isn't tested atm.

@JPBergsma JPBergsma changed the title Queries with the form: value != prop return entries where prop = None Queries with the form: 'value != prop' return entries where 'prop == None' Apr 28, 2022
@JPBergsma
Copy link
Contributor Author

JPBergsma commented Apr 28, 2022

There is an automatically generated test in _construct_single_property_filters in validator.py.
I doubt it is tested properly now, as none of the references that are used for testing lack a field that another references has. So we should probably add/modify the test data rather than make a new test.

The property first query indeed passes the line of code you mentioned.
This is however not the case for the value first query. I still have to figure out why.

I did notice that the lark parser seems to interpret each of these cases differently, although I am not sure whether this is the cause of the problem. (I left the part of the tree, that is the same for both, out)

Incase of '"10.1145/362929.362947" != doi' the lark parser interprets it as : constant_first_comparison(constant(string("10.1145/362929.362947")), token(!=), non_string_value('property'(doi))

Incase of 'doi != "10.1038/335201a0"' the lark parser interprets it as : 'property_first_comparison'('property'(doi),'value_op_rhs'(!=,'value'('string'("10.1038/335201a0"))))

@ml-evs
Copy link
Member

ml-evs commented Apr 28, 2022

Yeah, ideally we should be able to call property_first_comparison from within constant_first_comparison with the reversed operator, but I have a feeling I tried this at the time and it didn't work. I can investigate a bit over the next few days if you haven't already started.

@JPBergsma
Copy link
Contributor Author

I think I have an idea of how to solve it.
I could make a separate function which I can call from both property_first_comparison and constant_first_comparison.
I'll try to make that today.

@JPBergsma
Copy link
Contributor Author

I noticed that handling the $ne and the $size operator in property_first_comparison can not be handled both at the same time. I was therefore wondering whether it is forbidden to have a query like ?filter=elements LENGTH != 3

I also seem to be able to call property_first_comparison from constant_first_comparison so that may be an even better solution.

@JPBergsma JPBergsma added bug Something isn't working transformer/MongoDB Related to filter transformer for MongoDB labels Apr 29, 2022
@ml-evs
Copy link
Member

ml-evs commented Apr 29, 2022

I noticed that handling the $ne and the $size operator in property_first_comparison can not be handled both at the same time. I was therefore wondering whether it is forbidden to have a query like ?filter=elements LENGTH != 3

This is a MongoDB limitation, you can't do $size queries with comparison operators, only equality. If the field has a length alias (like elements), then the query will go ahead and query on the aliased integer field nelements without raising this error.

We could implement $ne and $size in the same we did $lt and $size as a post-processing of the transformed filter that checks for the existence of the i-th value in a list (e.g., elements LENGTH > 4 becomes "does elements[4] exist?"). For $ne you would have to check whether the length is greater than or less than the target value. Given that all array fields within OPTIMADE currently have length aliases (and they are trivial to define) I'm not sure how much effort I would recommend you put into this!

@JPBergsma
Copy link
Contributor Author

I have put it on my private low priority to-do list. Probably most of the things on that list will however never get done.
There is of course also a workaround, as you can specify that LENGTH should be larger or smaller than a certain value.

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
Projects
None yet
2 participants