-
Notifications
You must be signed in to change notification settings - Fork 26
add xor support for Q objects #81
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
Conversation
@@ -219,8 +222,21 @@ def where_node(self, compiler, connection): | |||
if self.connector == AND: | |||
operator = "$and" | |||
elif self.connector == XOR: | |||
# https://github.com/mongodb-labs/django-mongodb/issues/27 | |||
raise NotImplementedError("XOR is not yet supported.") | |||
# MongoDB doesn't support $xor, so convert: |
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.
Another way to compute XOR is by recognizing that XOR is equivalent to "not equal" ($ne). Therefore, the expression a xor b xor c can be interpreted as (a $ne b) $ne c. Whit this we can avoid checking parity.
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.
I think it was originally implemented like that, but it doesn't work with multiple conditions: django/django#14480 (comment).
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.
Just to check, this is as it relates to logical operators and not for numerical calculations?
If so, then I wholeheartedly agree.
Example :
>> XOR(2,5) = 7 # True
>> XOR(2,2) = 0 # False
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.
Yes, you can look at the new functionality this enables: https://github.com/django/django/blob/main/tests/xor_lookups/tests.py
bitXor (tests) is already implemented: https://github.com/mongodb-labs/django-mongodb/blob/6b18784e443c93c80d317812c9cf0055e1333498/django_mongodb/operations.py#L27
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.
I think I cannot read the commits, but the idea is not use and as a nexo between operators.
I am thinking of:
# (((a != b) != c) != ... )
# I think we cannot create a not Exact so easily. but we have ~a ≡ a == False => Exact(Exact(a, b), False) will generate a not equal.
pred = Value(False) # is the neutro of the xor and base case of XOR quantifier
for c in self.children:
pred = Exact(Exact(pred, c), Value(False))
return self.__class__((lhs, pred), AND, self.negated).as_mql(compiler, connection)
The other idea is, changing the line when mongo-operator is used, and doing something like:
if len(children_mql) == 1:
mql = children_mql[0]
elif len(children_mql) > 1:
if self.connector == XOR:
mql = {"$literal": False}
for prop in children_mql:
mql = {"$ne": [mql, prop]}
else:
mql = {operator: children_mql} if children_mql else {}
else:
mql = {}
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.
The first suggestion doesn't pass xor_lookups.tests.XorLookupsTests.test_empty_in
. No results are returned for Number.objects.filter(Q(pk__in=[]) ^ Q(num__gte=5))
, probably because Q(pk__in=[])
raises EmptyResultSet
short-circuiting the entire expression.
The second suggestion passes the tests. It's less clear to me how to translate that to Django's WhereNode
, if we wanted to suggest that implementation upstream.
Is the main advantage you see in your suggestion a simpler query without Case/When?
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.
Well, the advantage is not much, question of taste.
The other idea is:
# I think we cannot create a not Exact so easily. but we have a!= b ≡ -a = b
pred = Value(False) # is the neutro of the xor and base case of XOR quantifier
for c in self.children:
pred = Exact(pred, Case(When(c, then=False), default=True))
return self.__class__((lhs, pred), AND, self.negated).as_mql(compiler, connection)
The case catch the empty of full results.
Note that the c is negated in the when.
As I said above, question of taste. The PR is good to me. Ignore this if you think the other way is simpler.
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.
I prefer to copy Django's implementation so if something comes up later, we can (probably) just copy in those changes.
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.
LGTM, if you want, we can change the way that XOR is computed
The implementation is adapted from Django's
WhereNode.as_sql()
.fixes #27