Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Predicate functionality for Q objects #388

Closed
wants to merge 46 commits into from

4 participants

ptone added some commits
@ptone ptone initial implementation of predicate into Q 3854ad7
@ptone ptone move manager check, add match_compile api 05dd71e
@ptone ptone typo fix 1551a42
@ptone ptone resolve a circular import 4b80f05
@ptone ptone clean up recursion ebbde80
@ptone ptone fix call to super.__init__ 5612d95
@ptone ptone more correctly match add_filter
since we aren't interested in values during traversal
0a7d769
@ptone ptone handle the case when related objects don't exist a11f1d6
@ptone ptone add initial tests b90bbe1
@ptone ptone Merge branch 'master' into q-predicate
Conflicts:
	django/db/models/sql/query.py
	updates location of lookup_sep constant
9a2febc
@ptone ptone use new lookup_sep constant location 6e92d8b
@ptone ptone fix up traversal 0dc4d52
@ptone ptone add more tests b9101d4
@ptone ptone initial gis test structure f73d053
@ptone ptone start on geodjango tests d031a86
@ptone ptone remove bbox helper 3484332
@ptone ptone updating distance tests with stubs 310b0db
@ptone ptone mostly testing stubs 5a7064c
@ptone ptone remove separate testing app no longer used 66285cc
@ptone ptone gis spatial predicates with tests b51e79d
@ptone ptone start on distances implementation b0ce713
@ptone ptone Fixed #18971 -- added root level CONTRIBUTING doc b9faeb2
@ptone ptone added distance lookups with tests f738405
@ptone ptone Merge branch 'q-predicate' of /Users/preston/Projects/code/django-con…
…tributing/test-vagrant/../../forks/django into q-predicate
cec603a
@ptone ptone add fix and test for bad field name f818406
@ptone ptone added alternate manager tests 052daf0
@ptone ptone initial pass at docs 934f5c6
@ptone ptone added release note f3732bd
@ptone ptone doc revision 9e27855
@ptone ptone Merge branch 'master' into q-predicate d9cf3d3
@ptone ptone add negation handling and tests 003b372
@ptone ptone finished wrong manager tests d742df8
@ptone ptone finished first pass at docs a9dc02e
@ptone ptone pep8 and cleanups b5b4b28
django/contrib/gis/db/models/sql/matching.py
((15 lines not shown))
+def _contains(model, instance_value, value):
+ return instance_value.contains(value)
+
+
+def _contained(model, instance_value, value):
+ instance_bbox = Polygon.from_bbox(instance_value.extent)
+ value_bbox = Polygon.from_bbox(value.extent)
+ return value_bbox.contains(instance_bbox)
+
+
+def _contains_properly(model, instance_value, value):
+ return instance_value.relate_pattern(value, 'T**FF*FF*')
+
+
+def _coveredby(model, instance_value, value):
+ # T*F**F***, *TF**F***, **FT*F***, **F*TF***
@alex Collaborator
alex added a note

This comment is pretty useless

@ptone Collaborator
ptone added a note

Yes - it was a note as to the GEO matrix options I wanted to cover, copied and pasted from a postgis or similar docs page - will strike

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/contrib/gis/db/models/sql/matching.py
((22 lines not shown))
+ return value_bbox.contains(instance_bbox)
+
+
+def _contains_properly(model, instance_value, value):
+ return instance_value.relate_pattern(value, 'T**FF*FF*')
+
+
+def _coveredby(model, instance_value, value):
+ # T*F**F***, *TF**F***, **FT*F***, **F*TF***
+ return any((
+ instance_value.relate_pattern(value, 'T*F**F***'),
+ instance_value.relate_pattern(value, '*TF**F***'),
+ instance_value.relate_pattern(value, '**FT*F***'),
+ instance_value.relate_pattern(value, '**F*TF***'),
+ ))
+
@alex Collaborator
alex added a note

Use or here, not any, same for all the other places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/contrib/gis/db/models/sql/query.py
@@ -21,6 +22,13 @@
])
ALL_TERMS.update(sql.constants.QUERY_TERMS)
+ALL_MATCHES = sql.matching.match_functions
+
+# we update match functions in the reverse of query_terms, as we want the
+# gis version to be the one in the final lookup ie 'contains' should be gis
+# contains
+ALL_MATCHES.update(match_functions)
+
@alex Collaborator
alex added a note

This is mutating the external primary sql match functions, that shouldn't be necessary.

@ptone Collaborator
ptone added a note

is now mutating a local copy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/query_utils.py
@@ -41,7 +43,9 @@ class Q(tree.Node):
default = AND
def __init__(self, *args, **kwargs):
- super(Q, self).__init__(children=list(args) + list(six.iteritems(kwargs)))
+ super(Q, self).__init__(children=list(args)
+ + list(six.iteritems(kwargs)))
@alex Collaborator
alex added a note

This line spacing is really awkward, please don't make unrelated changes like this, especially when they make stuff uglier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/db/models/query_utils.py
((15 lines not shown))
+
+ parent.children.append(branch_root)
+ descend(branch_root, child.children)
+ else:
+ # assuming we are in a properly formed Q, could only be
+ # a tuple
+ child_le = LookupExpression(expr=child, manager=manager)
+ parent.children.append(child_le)
+
+ root = LookupExpression(connector=self.connector, manager=manager)
+ descend(root, self.children)
+ root.negated = self.negated
+
+ self._compiled_matcher = root
+
+ def matches(self, instance, manager=None):
@alex Collaborator
alex added a note

Whatever manager is needs to be documented, or it shouldn't be a parameter on a public API.

@ptone Collaborator
ptone added a note

yes - it is documented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/query_utils.py
((34 lines not shown))
+ if manager is None:
+ manager = instance._default_manager
+
+ if not self._compiled_matcher:
+ # we are evaluating the first model, or are uncompiled
+ self._compile_matcher(manager)
+ if self._compiled_matcher.manager != manager:
+ # the pre-compiled matcher was compiled for a different manager
+ self._compile_matcher(manager)
+ return_val = self._compiled_matcher.matches(instance)
+ if self.negated:
+ # It is extremely unlikely (impossible?) to end up with a root
+ # Q object that is negated, given the implementation of __invert__
+ # we should be able to return _compiled_matcher.matches() directly
+ # but in case we encounter an unforseen edgecase, we check again
+ return not return_val
@alex Collaborator
alex added a note

Either it's possible or it's not, if you can write a test case for it to happen it's possible and this comment is confusing, if not this condition should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/query_utils.py
((58 lines not shown))
+ self.lookup_type = parts.pop()
+ return
+ # Unless we're at the end of the list of lookups, let's attempt
+ # to continue traversing relations.
+ if (counter + 1) < num_parts:
+ try:
+ lookup_model = lookup_field.rel.to
+ except AttributeError:
+ # Not a related field. Bail out.
+ self.lookup_type = parts.pop()
+ return
+ else:
+ # presumably we have a simple <field>=x with no lookup term
+ # so we just use our default of exact
+ self.attr_route.append(parts[0])
+ return
@alex Collaborator
alex added a note

This function should return some values to be checke by its caller, not set things on self.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/query_utils.py
((80 lines not shown))
+
+ def matches(self, instance):
+ """
+ Evaluates an instance against the lookup we were created with.
+ Return true if the instance matches the condiiton.
+ """
+ if not isinstance(instance, self.manager.model):
+ raise ValueError("invalid manager given for {}".format(instance))
+
+ evaluators = {"AND": all, "OR": any}
+ evaluator = evaluators[self.connector]
+ return_val = None
+ if self.children:
+ return_val = (
+ evaluator(c.matches(instance) for c in self.children)
+ )
@alex Collaborator
alex added a note

Drop the parens and put this all on one line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/query_utils.py
((95 lines not shown))
+ )
+ else:
+ try:
+ instance_value = self.get_instance_value(instance)
+ return_val = self.lookup_function(instance, instance_value,
+ self.value)
+ except AttributeError:
+ # this is raised when we were not able to traverse the full
+ # attribute route. In nearly all cases this means the match
+ # failed as it specified a longer relationship chain then
+ # exists for this instance.
+ if (hasattr(self.lookup_function, 'none_is_true')
+ and self.lookup_function.none_is_true):
+ return_val = True
+ else:
+ return_val = False
@alex Collaborator
alex added a note

return_val = getattr(self.lookup_function, 'none_is_true', False)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/sql/matching.py
((70 lines not shown))
+ return instance_value.day == value
+
+
+def _week_day(model, instance_value, value):
+ return instance_value.weekday() == value
+
+
+def _isnull(model, instance_value, value):
+ if value:
+ return instance_value is None
+ else:
+ return instance_value is not None
+
+# This is a special attr/flag to designate that when None is the instance_value
+# due to an inability to follow a set of relationships, True should be returned
+# for the match, as in most cases, the match would be considered False
@alex Collaborator
alex added a note

I can't understand this comment. If I can't I doubt anyone else will be able to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex
Collaborator

Overall the patch is ok, but ISTM this should really wait for a cleanup of the __ lookup stuff, because right now this is just adding more code that only works with the fixed set of builtin lookups, rather than moving us towards being able to have custom lookups, as it is the GIS stuff is a bit of a hack just monkeypatching the builtin dict of matchers.

docs/releases/1.5.txt
@@ -90,6 +90,13 @@ In all :doc:`generic class-based views </topics/class-based-views/index>`
(or any class-based view inheriting from ``ContextMixin``), the context dictionary
contains a ``view`` variable that points to the ``View`` instance.
+Use of Q objects as a predicate test for model instances
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Q objects now provide a :meth:`~django.db.models.Q.matches()
+<django.db.models.Q.matches>` method that will determine whether matches the
@apollo13 Owner

"Whether matches the" -- missing "the instance" there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/topics/q-objects.txt
((127 lines not shown))
+
+.. method:: Q.matches(instance, [manager=None])
+
+ Returns ``True`` or ``False`` based on whether the instance matches the
+ condition specified by the Q object. If an object specifies multiple
+ managers, a specific manager can be specified, otherwise the default
+ manager is used. The need to specify a specific manager is uncommon - see
+ details on alternate managers below.
+
+.. note::
+
+ When negating a Q object, the result is a new Q object, it is not negated
+ in place. That means it is invalid to both negate a Q object and call
+ matches on it in a simple statement. ie this won't work as expected:
+ ``~my_q.matches(some_instance)``. Instead, assign the negated Q to a new
+ local variable, then call the matches method on that.
@apollo13 Owner

I don't like assigning to a new variable, (~qobject).matches should work too. And the issue is IMO not that a new Q object is created but that attribute look up comes before bitwise not in the operator precedence list.

@ptone Collaborator
ptone added a note

agree - revising

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/topics/q-objects.txt
((146 lines not shown))
+ When to use Q objects to test against some set of objects instead of
+ issuing a separate query to the database will depend on a number of
+ factors, including the number of different conditions you are testing, the
+ number of objects to be tested, and whether a QuerySet may already be
+ evaluated. Profiling and architectural necessity will be your best guides
+ in making the right choice.
+ (https://docs.djangoproject.com/en/dev/ref/models/querysets/#when-querysets-are-evaluated)
+
+Match Compiling and alternate managers
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. note::
+
+ This section covers API details that aren't commonly used.
+
+Testing whether an instance matches)a condition requires evaluating the lookups
@apollo13 Owner

s/)/ /

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@akaariai
Collaborator

Some quick comments about this feature on topical level:

  • In general it will be impossible to make sure that filtering in Python works exactly like it does in SQL. Things like case insensitive matches are locale and DB vendor specific.
  • Many fields/lookups do type conversion to values. An example is Q(date=date.today()).matches(SomeModel(ldate=datetime.now())). If the SomeModel is saved to DB, the model matches (the datetime is converted to date on save). But, as a Python lookup it doesn't.
  • The usual suspects: How does this work for reverse relations, generic foreign keys and models with parents/childs? How about timezone stuff?

In general I am suspicious of claiming that this does the same thing as the lookup would do in ORM. For that reason I would separate this from Q-object. You would compile an obj-matcher from the q-object, and do the matching using that. The matcher object could be documented as doing best-effort matching in Python. The biggest reason for the separation is that this way the expectation that compile_matcher(q).matches(a_model) does the exact same thing as the actual ORM implementation is not so strong.

As for the implementation: The biggest worry for me is the lookup traversal code. I am surprised if that does what the ORM lookup traversal does. Also, this should be tied more tightly to fields so that one could do the type conversions per field.

My initial opinion is that we might want this into core, but this needs to be clearly documented as something not even trying to do the exact same thing as the lookup will do when ran through ORM. We will never get this to do the exact same thing as the ORM lookup does, so lets not claim that.

@ptone
Collaborator

responding to @akaariai:

First, thanks for the review. I agree that this should be clearly documented as not mirroring the backend behavior 100%. This was originally something outside of Q, but it was suggested by both @jacobian and @malcolmt that it probably belongs as a feature of Q. Because this deals with Django models, but not with any db backend, this exists as a bit of a chimera inside the ORM, but doesn't have anywhere better to live in Django, as for the most part, model = ORM.

The datetime vs date conversion is an issue - but my hunch is that there should be a way to call the field's to_python method - I'm looking into it, and have a failing test described by your situation to work with.

Ideally the value of the field as accessed by simple attribute should be the same on the newly created instance, as for the fetched one. So the situation you raise I think is problematic for more situations than just Q as predicate, but for other potential situations. In general the model should give you the right value when asked - so in the case of your 'usual suspects' it shouldn't be the concern of the q-lookup code, that should be properly handled by the attribute access on the model. The compiling step of Q simply validates a chain of relationships and field name as being a valid attribute traversal path.

ptone added some commits
@ptone ptone use more field centric conversions
for both the lookup value, and the instance value
field specific methods are used to effect any conversions
7bdf334
@ptone ptone start on reverse relations ba58c8f
@akaariai
Collaborator

If others have voted to keep this as q-obj functionality, then lets do that. The main point is that lets not advertise this as interchangeable of doing the matching in the DB.

As for reverse lookups, I don't believe this works yet as ORM lookups do. If you have a lookup like Q(friends__age__gte=20)&Q(friends__age__lte=30) the filter must target the same friend of the possibly many friends. This is something the patch should handle, or explicitly throw an error and say that this isn't handled yet. I am not sure if this already works, but at least it looks like this isn't tested. (This is a somewhat strange interpretation, if you have two friends, one aged 19, other 31, the filters will match individually, but not ANDed... but that is what the ORM does)

I am fine with leaving some of the functionality for later, as long as the non-working cases are errors and documented.

I wonder if to_python() is the correct method to use for type conversions. IIRC it can throw ValidationError which seems like a strange error to get from q_object.matches(someobj). Maybe catch and throw TypeError? A better approach might be to define a field method purely for this. This could of course hook to existing methods by default.

In general if this gets merged I expect this to cause a lot of little bugs in type conversions and in trying to match exactly what the ORM does. This feature is likely worth the needed fixing effort.

My feeling is that this will likely not make it into 1.5. I am not saying it is impossible, but to me it seems there is still too much to do, and in addition review & committer time is in short supply.

@ptone
Collaborator

@akaariai thanks again for the valuable feedback. I have what I think is a technically functional strategy for the reverse relations part - but it is poorly optimized and I'd be hard pressed to think of a case where it would be preferred over testing membership of your instance in a queryset. An alternate method to to_python might make sense, but could probably come up with a cleanup of the way fields and lookups work in general.

So yes - starting to get the feeling that this is perhaps in the "close, but no cigar" category for 1.5 :-/

@ptone
Collaborator

Postponed for now - pending improvements in field based lookups

@ptone ptone closed this
@ptone ptone referenced this pull request in ptone/django-predicate
Open

Merge downstream changes into django-predicate #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 8, 2012
  1. @ptone
  2. @ptone
  3. @ptone

    typo fix

    ptone authored
  4. @ptone

    resolve a circular import

    ptone authored
Commits on Sep 9, 2012
  1. @ptone

    clean up recursion

    ptone authored
  2. @ptone

    fix call to super.__init__

    ptone authored
  3. @ptone

    more correctly match add_filter

    ptone authored
    since we aren't interested in values during traversal
  4. @ptone
  5. @ptone

    add initial tests

    ptone authored
  6. @ptone

    Merge branch 'master' into q-predicate

    ptone authored
    Conflicts:
    	django/db/models/sql/query.py
    	updates location of lookup_sep constant
  7. @ptone
Commits on Sep 11, 2012
  1. @ptone

    fix up traversal

    ptone authored
  2. @ptone

    add more tests

    ptone authored
  3. @ptone

    initial gis test structure

    ptone authored
Commits on Sep 12, 2012
  1. @ptone

    start on geodjango tests

    ptone authored
  2. @ptone

    remove bbox helper

    ptone authored
  3. @ptone
  4. @ptone

    mostly testing stubs

    ptone authored
  5. @ptone
Commits on Sep 17, 2012
  1. @ptone
  2. @ptone
Commits on Sep 19, 2012
  1. @ptone
Commits on Sep 20, 2012
  1. @ptone
  2. @ptone

    Merge branch 'q-predicate' of /Users/preston/Projects/code/django-con…

    ptone authored
    …tributing/test-vagrant/../../forks/django into q-predicate
Commits on Sep 21, 2012
  1. @ptone
  2. @ptone

    added alternate manager tests

    ptone authored
  3. @ptone

    initial pass at docs

    ptone authored
Commits on Sep 22, 2012
  1. @ptone

    added release note

    ptone authored
  2. @ptone

    doc revision

    ptone authored
  3. @ptone
  4. @ptone

    add negation handling and tests

    ptone authored
  5. @ptone

    finished wrong manager tests

    ptone authored
  6. @ptone

    finished first pass at docs

    ptone authored
  7. @ptone

    pep8 and cleanups

    ptone authored
Commits on Sep 23, 2012
  1. @ptone

    removed some extraneous comments

    ptone authored
  2. @ptone
  3. @ptone
  4. @ptone
  5. @ptone
  6. @ptone
  7. @ptone
  8. @ptone

    fixed and improved release note

    ptone authored
  9. @ptone
  10. @ptone

    whitespace cleanup

    ptone authored
Commits on Sep 27, 2012
  1. @ptone

    use more field centric conversions

    ptone authored
    for both the lookup value, and the instance value
    field specific methods are used to effect any conversions
  2. @ptone

    start on reverse relations

    ptone authored
Something went wrong with that request. Please try again.