add support for queries with joins #63

WaVEV · 2024-06-28T23:35:14Z

Fixes #35

timgraham · 2024-07-02T20:43:33Z

django_mongodb/features.py

@@ -70,6 +72,8 @@ class DatabaseFeatures(BaseDatabaseFeatures):
        "model_fields.test_jsonfield.TestQuerying.test_order_grouping_custom_decoder",
        "model_fields.test_jsonfield.TestQuerying.test_ordering_by_transform",
        "model_fields.test_jsonfield.TestQuerying.test_ordering_grouping_by_key_transform",
+        # Ordering 'OrderBy' is not iterable


There is an existing section above: "Lookup in order_by() not supported"

timgraham · 2024-07-02T20:45:03Z

django_mongodb/operations.py

@@ -159,6 +159,12 @@ def execute_sql_flush(self, tables):
            if not options.get("capped", False):
                collection.drop()

+    def prepare_join_on_clause(self, lhs_table, lhs_field, rhs_table, rhs_field):


Isn't this the same implementation as BaseDatabaseOperations?

I thought that this implementation was in Postgres's backend. So I will remove.

timgraham · 2024-07-02T20:49:11Z

django_mongodb/datastructures.py

@@ -0,0 +1,75 @@
+from django.db.models.sql.constants import INNER


I'm not sure if datastructers.py is the best name. Do you anticipate we need to modify any of the other classes in that file? If not, I might suggest join.py, or even put this in query.py between MongoQuery and where_node? No need to make a decision immediately.

I don't think that we will modify BaseTable, I will move it to query.

timgraham · 2024-07-02T20:50:24Z

django_mongodb/compiler.py

@@ -202,8 +220,24 @@ def _get_ordering(self):
            field_ordering.append((opts.get_field(name), ascending))
        return field_ordering

+    @property


Might want to use from django.utils.functional import cached_property.

timgraham · 2024-07-02T20:54:32Z

django_mongodb/compiler.py

+            result = tuple(result)
+        return result
+
+    def _apply_converters(self, entity, columns, converters):


I'd be in favor of a refactor to add def apply_converters since that's consistent with the API of Django's SQLCompiler. I tried to refactor this to add that and get rid of _make_result() (which came from django-nonrel) but wasn't successful. Anyway, is it a necessary part of this PR? It looks like it was initially but may no longer be. If possible, I'd try to keep cosmetic cleanups in a separate PR from functionality changes.

timgraham · 2024-07-02T20:55:48Z

README.md

@@ -114,7 +114,6 @@ Migrations for 'admin':
  - `datetimes()`
  - `distinct()`
  - `extra()`
-  - `select_related()`

 - Queries with joins aren't supported.


This is also obsolete, kind of. I guess we're in a weird state where some cases are supported. Anyway, we don't need to update the documentation at every stage if this is going to evolve through a series of PRs.

django_mongodb/compiler.py

timgraham · 2024-07-02T21:00:25Z

django_mongodb/query.py

        pipeline = []
+        if self.mongo_lookups:
+            lookups = self.mongo_lookups


the lookups variable seems unnecessary (possibly a debugging or refactoring artifact)

timgraham · 2024-07-02T21:03:19Z

django_mongodb/query.py

+        # maybe I have to create a new object or named tuple.
+        # it will save lookups, some filters (in case of inner) and project to rename field
+        # don't know if the rename is needed
+        self.mongo_lookups = None


I'm not sure if mongo_lookups is the best name. It kind of confuses me with Django lookups / lookups.py. Maybe something like lookup_pipeline?

timgraham · 2024-07-02T21:03:51Z

django_mongodb/query.py

@@ -102,7 +107,17 @@ def get_cursor(self):
                # If name != column, then this is an annotatation referencing
                # another column.
                fields[name] = 1 if name == column else f"${column}"
+
+        # Add the subquery results if fields is defined.


Is "subquery" the best word choice?

No, I changed it for related.

timgraham · 2024-07-05T18:00:38Z

django_mongodb/query.py

+    lhs_fields = []
+    rhs_fields = []
+    for lhs, rhs in join_fields:
+        if isinstance(lhs, str):


What case is this? I haven't been able to find the tests that use this branch.

I imported the logic from https://github.com/WaVEV/django/blob/main/django/db/models/sql/datastructures.py#L88
In that join function there is a second part of the join, but I didn't find where it is used.

Thanks. So it's to support a deprecated private API. I think we should just simplify the code now and not worry about this. It's untested by Django's test suite anyway (checked running with PostgreSQL). Any third-party apps should be on their way to supporting the new API by now.

django_mongodb/query.py

timgraham · 2024-07-05T18:13:30Z

django_mongodb/query.py

+        rhs_fields.append(rhs_mql)
+
+    parent_template = "parent__field__"
+    lookups_pipeline = [


comment for this?

WaVEV · 2024-07-06T02:51:49Z

django_mongodb/compiler.py

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


Should this method be private?

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.

Jibola

LGTM! Adding some questions to get clarification.

Jibola · 2024-07-03T13:42:05Z

django_mongodb/expressions.py

+    prefix = f"{self.alias}." if self.alias != compiler.collection_name else ""
+    return f"${prefix}{self.target.column}"


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

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

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.

Jibola · 2024-07-08T15:11:11Z

django_mongodb/compiler.py

+
+    def get_lookup_pipeline(self):
+        result = []
+        for alias in tuple(self.query.alias_map):


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

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

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.

Jibola · 2024-07-08T15:11:50Z

django_mongodb/features.py

+        # Wrong results in queries with multiple tables.
+        "annotations.tests.NonAggregateAnnotationTestCase.test_annotation_aggregate_with_m2o",
+        "annotations.tests.NonAggregateAnnotationTestCase.test_annotation_reverse_m2m",
+        "annotations.tests.NonAggregateAnnotationTestCase.test_chaining_annotation_filter_with_m2m",
+        "lookup.tests.LookupTests.test_lookup_collision",
+        "expressions_case.tests.CaseExpressionTests.test_join_promotion",
+        "expressions_case.tests.CaseExpressionTests.test_join_promotion_multiple_annotations",
+        "ordering.tests.OrderingTests.test_order_by_grandparent_fk_with_expression_in_default_ordering",
+        "ordering.tests.OrderingTests.test_order_by_parent_fk_with_expression_in_default_ordering",
+        "ordering.tests.OrderingTests.test_order_by_ptr_field_with_default_ordering_by_expression",


In what way are these results wrong?

I think there is more than one issue represented here. I'll investigate in more detail later but don't want to hold up the merge for that.

Also fixes mongodb#35: add support for QuerySet.select_related()

WaVEV requested review from timgraham and Jibola June 28, 2024 23:35

WaVEV marked this pull request as draft June 28, 2024 23:35

WaVEV changed the title ~~Support lookup from related collections (#1)~~ Support lookup from related collections Jun 29, 2024

WaVEV marked this pull request as ready for review July 1, 2024 01:50

WaVEV marked this pull request as draft July 1, 2024 03:05

WaVEV force-pushed the support-lookup-from-related-collections branch from 9915c05 to 60e7eb1 Compare July 1, 2024 15:02

WaVEV marked this pull request as ready for review July 2, 2024 05:11

WaVEV force-pushed the support-lookup-from-related-collections branch from 0a6316e to 8ec51cd Compare July 2, 2024 05:41

timgraham reviewed Jul 2, 2024

View reviewed changes

WaVEV force-pushed the support-lookup-from-related-collections branch 3 times, most recently from 52a2d47 to 91e9b3a Compare July 5, 2024 15:26

timgraham reviewed Jul 5, 2024

View reviewed changes

timgraham changed the title ~~Support lookup from related collections~~ add support for queries with joins Jul 6, 2024

WaVEV commented Jul 6, 2024

View reviewed changes

timgraham approved these changes Jul 8, 2024

View reviewed changes

Jibola approved these changes Jul 8, 2024

View reviewed changes

add support for queries with joins

5e77b83

Also fixes mongodb#35: add support for QuerySet.select_related()

timgraham force-pushed the support-lookup-from-related-collections branch from 8a2fe23 to 5e77b83 Compare July 9, 2024 13:25

timgraham merged commit 5e77b83 into mongodb:main Jul 9, 2024
3 checks passed

		@@ -0,0 +1,75 @@
		from django.db.models.sql.constants import INNER

		prefix = f"{self.alias}." if self.alias != compiler.collection_name else ""
		return f"${prefix}{self.target.column}"

add support for queries with joins #63

add support for queries with joins #63

Uh oh!

Conversation

WaVEV commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jibola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

WaVEV commented Jun 28, 2024 •

edited

Loading