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

[Do Not Merge] Add support for multi column joins Ticket 19385 #656

Closed
wants to merge 11 commits into from

Conversation

Projects
None yet
3 participants
@jtillman
Copy link

commented Jan 17, 2013

This is an operating but not final version. Still needs some polishing before merging. Please review and comment with questions/concerns.

-JeremyT

Jeremy Tillman added some commits Dec 13, 2012

@akaariai

View changes

django/db/models/fields/related.py Outdated
@@ -7,7 +7,7 @@
PositiveIntegerField, PositiveSmallIntegerField, FieldDoesNotExist)
from django.db.models.related import RelatedObject, PathInfo
from django.db.models.query import QuerySet
from django.db.models.query_utils import QueryWrapper
from django.db.models.query_utils import Q, QueryWrapper

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

Q never used

This comment has been minimized.

Copy link
@jtillman

jtillman Jan 18, 2013

Author

Good catch... I'll remove. I'm used to having git hooks that catch these on commit.

@akaariai

View changes

django/db/models/fields/related.py Outdated
root_constraint.add(SubqueryConstraint(alias, columns, [target.name for target in targets], raw_value), AND)
elif lookup_type == 'isnull':
root_constraint.add((Constraint(alias, columns[0], None), lookup_type, raw_value), AND)
elif lookup_type in ['exact', 'gt', 'lt', 'gte', 'lte']:

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

I don't think you can do it like this for multicolumn lt, gt constraints. The natural constraint is:
a < val1 or (a == val1 and b < val2)
at least that is what PostgreSQL gives you.

EDIT: We can just throw an error in multicolumn non-exact lookups here for now.

This comment has been minimized.

Copy link
@jtillman

jtillman Jan 18, 2013

Author

I can't take this out because we use the ForeignKey field as the join field. So basically a column reference and a join reference in the same field. Doing 'foreignfield_id' > 5 maps you to 'foreignfield' > 5. This is symmetrical to what used to be in 'RelatedField.get_db_prep_lookup'. If you would like, I can raise an exception in the multicolumn case.

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

Yeah, multicolumn case is what I am interested in, the results will not be correct for cases where the first column match, the second is smaller and we use __lt. So, error out in multicolumn case for now, then lets think if we can make this work properly (for some DBs the DB itself knows how to implement (a, b) < (val1, val2)).

@akaariai

View changes

django/db/models/fields/related.py Outdated
else:
target = opts.pk
return pathinfos, opts, target, self
pathinfos = [PathInfo(from_opts, opts, (opts.pk,), self, not self.unique, False)]

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

why opts.pk here? Not saying this is incorrect, but I just don't see immediately why this is a correct replacement for the above. The old code above was copy-paste from old code in sql/query.py, and I never got to the point of actually understanding what it is doing.

This comment has been minimized.

Copy link
@jtillman

jtillman Jan 25, 2013

Author

This shouldn't make any resulting difference from query results. I'm unsure what the hook originally did but I've added it back just in case.

@akaariai

View changes

django/db/models/fields/related.py Outdated
return [(rhs_field, lhs_field) for lhs_field, rhs_field in self.related_fields]

@property
def native_related_fields(self):

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

I can't see this used anywhere, deadcode?

@akaariai

View changes

django/db/models/sql/where.py Outdated
@@ -9,7 +9,9 @@
from itertools import repeat

from django.utils import tree
from django.db import connections

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

unused import

hasattr(raw_value, 'get_compiler'):
root_constraint.add(SubqueryConstraint(alias, columns, [target.name for target in targets], raw_value), AND)
elif lookup_type == 'isnull':
root_constraint.add((Constraint(alias, columns[0], None), lookup_type, raw_value), AND)

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

I don't know of this particular case, but I wonder if we will have a fun time ahead regarding NULL handling in general - partial match foreign keys etc, and what it in general means for a composite field to be null... There are some similar cases in Query.add_filter() negated handling.

col, alias, join_list = self.trim_joins(target, join_list, path)
cols, alias, join_list = self.trim_joins(targets, join_list, path)

if hasattr(field, 'get_lookup_constraint'):

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

Just to make sure I understand this code - we end up in get_lookup_constraint branch in the case the final_field was a ForeignKey. If so, we do the _pk_trace stage in get_lookup_constraint().

How about the get_db_prep_lookup(), get_prep_lookup()? If I understand correctly, these seem to be handled by the underlying fields now which is of course the right thing to do. It might be we need to document this as backwards incompatible change for those who have subclassed ForeignKey and overridden these public methods.

This comment has been minimized.

Copy link
@jtillman

jtillman Jan 25, 2013

Author

Correct... So no Model instance will ever make it to 'get(_db)_prep_lookup'. It will only receive the value from it from now on. I've added back 'get(_db)_prep_lookup' to return the values of the related_fields function.

self.targets = targets
self.query_object = query_object

def as_sql(self, qn, connection):

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

I don't particularly like this method - it seems we need to know too much internals of the query/compiler setup. If it is possible to write this so that compiler or query handles the dirty details that might be nicer. If not, well, then so be it...

This comment has been minimized.

Copy link
@jtillman

jtillman Jan 18, 2013

Author

I agree with you here as I would like to do the row constructor in case of mysql server instead of correlated subqueries.

This comment has been minimized.

Copy link
@jtillman

jtillman Jan 25, 2013

Author

Updated to delegate to the compiler. Also made mysql use row constructor format instead of dependent subquery


class SubqueryConstraint(object):
def __init__(self, alias, columns, targets, query_object):
self.alias = alias

This comment has been minimized.

Copy link
@akaariai

akaariai Jan 18, 2013

Member

How about alias relabeling? seems like that should be handled somehow.

This comment has been minimized.

Copy link
@jtillman

jtillman Jan 18, 2013

Author

Good catch... I'll remove.

@akaariai

This comment has been minimized.

Copy link
Member

commented Jan 18, 2013

Some generic notes:

  • whitespace errors (git diff --check master).
  • Running flake8 also recommended.
  • I really like the get_path_info() return value change, where the extra elements beside the pathinfo are dropped.
  • The RelatedField is a Field change seems to give some nice cleanup.

From above, if it is possible without huge effort to split the RelatedField is a Field and get_path_info() return value chages into separate pulls it would make life much easier for me when reviewing (at the cost of making life harder for you :P)

In general, I really like how this is turning out.

@timgraham timgraham closed this Jul 17, 2013

nanuxbe pushed a commit to nanuxbe/django that referenced this pull request Jul 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.