Skip to content

add support for queries with joins #63

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/test-python.yml
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,19 @@ jobs:
defer_regress
from_db_value
lookup
m2m_and_m2o
m2m_intermediary
m2m_multiple
m2m_recursive
m2m_regress
m2m_signals
m2m_through
m2o_recursive
model_fields
ordering
or_lookups
queries.tests.Ticket12807Tests.test_ticket_12807
select_related
sessions_tests
timezones
update
Expand Down
3 changes: 0 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,10 @@ Migrations for 'admin':
- `datetimes()`
- `distinct()`
- `extra()`
- `select_related()`

- `Subquery`, `Exists`, and using a `QuerySet` in `QuerySet.annotate()` aren't
supported.

- Queries with joins aren't supported.

- `DateTimeField` doesn't support microsecond precision, and correspondingly,
`DurationField` stores milliseconds rather than microseconds.

Expand Down
51 changes: 41 additions & 10 deletions django_mongodb/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from django.db.models.constants import LOOKUP_SEP
from django.db.models.sql import compiler
from django.db.models.sql.constants import GET_ITERATOR_CHUNK_SIZE, MULTI
from django.utils.functional import cached_property

from .base import Cursor
from .query import MongoQuery, wrap_database_errors
Expand Down Expand Up @@ -82,7 +83,15 @@ def _make_result(self, entity, columns, converters, tuple_expected=False):
result = []
for name, col in columns:
field = col.field
value = entity.get(name, NOT_PROVIDED)
column_alias = getattr(col, "alias", None)
obj = (
# Use the related object...
entity.get(column_alias, {})
# ...if this column refers to an object for select_related().
if column_alias is not None and column_alias != self.collection_name
else entity
)
value = obj.get(name, NOT_PROVIDED)
if value is NOT_PROVIDED:
value = field.get_default()
elif converters:
Expand Down Expand Up @@ -110,10 +119,6 @@ def check_query(self):
raise NotSupportedError("QuerySet.distinct() is not supported on MongoDB.")
if self.query.extra:
raise NotSupportedError("QuerySet.extra() is not supported on MongoDB.")
if self.query.select_related:
raise NotSupportedError("QuerySet.select_related() is not supported on MongoDB.")
if len([a for a in self.query.alias_map if self.query.alias_refcount[a]]) > 1:
raise NotSupportedError("Queries with multiple tables are not supported on MongoDB.")
if any(
isinstance(a, Aggregate) and not isinstance(a, Count)
for a in self.query.annotations.values()
Expand Down Expand Up @@ -147,6 +152,7 @@ def build_query(self, columns=None):
self.check_query()
self.setup_query()
query = self.query_class(self, columns)
query.lookup_pipeline = self.get_lookup_pipeline()
try:
query.mongo_query = {"$expr": self.query.where.as_mql(self, self.connection)}
except FullResultSet:
Expand All @@ -163,9 +169,17 @@ def get_columns(self):
columns = (
self.get_default_columns(select_mask) if self.query.default_cols else self.query.select
)
# Populate QuerySet.select_related() data.
related_columns = []
if self.query.select_related:
self.get_related_selections(related_columns, select_mask)
if related_columns:
related_columns, _ = zip(*related_columns, strict=True)

annotation_idx = 1
result = []
for column in columns:

def project_field(column):
nonlocal annotation_idx
if hasattr(column, "target"):
# column is a Col.
target = column.target.column
Expand All @@ -174,8 +188,13 @@ def get_columns(self):
# name for $proj.
target = f"__annotation{annotation_idx}"
annotation_idx += 1
result.append((target, column))
return tuple(result) + tuple(self.query.annotation_select.items())
return target, column

return (
tuple(map(project_field, columns))
+ tuple(self.query.annotation_select.items())
+ tuple(map(project_field, related_columns))
)

def _get_ordering(self):
"""
Expand Down Expand Up @@ -212,8 +231,20 @@ def _get_ordering(self):
field_ordering.append((opts.get_field(name), ascending))
return field_ordering

@cached_property
def collection_name(self):
return self.query.get_meta().db_table

def get_collection(self):
return self.connection.get_collection(self.query.get_meta().db_table)
return self.connection.get_collection(self.collection_name)

def get_lookup_pipeline(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this method be private?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it would be consistent with _get_ordering(). On the other hand, I'm not sure I would have underscore prefixed that method (that code came from the original fork). When I look at Django's SQL compiler, underscore methods are mainly used for utility methods used more than once in other methods.

This class doesn't have any methods Django developers call themselves, so whether or not to underscore is mainly a decision about how we want to organize things.

result = []
for alias in tuple(self.query.alias_map):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to cast this as a tuple before iterating on it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was copied from Django, but may be obsolete: django/django#18357

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an answer there, saying that the query.alias_map is modified during the for loops. 🤔 I didn't noticed that, maybe I have to check it.

if not self.query.alias_refcount[alias] or self.collection_name == alias:
continue
result += self.query.alias_map[alias].as_mql(self, self.connection)
return result


class SQLInsertCompiler(SQLCompiler):
Expand Down
4 changes: 3 additions & 1 deletion django_mongodb/expressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ def case(self, compiler, connection):


def col(self, compiler, connection): # noqa: ARG001
return f"${self.target.column}"
# Add the column's collection's alias for columns in joined collections.
prefix = f"{self.alias}." if self.alias != compiler.collection_name else ""
return f"${prefix}{self.target.column}"
Comment on lines +47 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we prefix the target column with the alias instead of using only the alias?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you misread it as "column's alias" instead of "column's collection's alias"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will explain the idea of the prefix:
When a collection is joined with the main collection, it generates a new "field" with the same as the joined collection (Now I am thinking if a collision is possible 🤔 ) this is why a prefix is computed if the column table (alias) is different from the current collection.



def combined_expression(self, compiler, connection):
Expand Down
Loading