add support for creating and deleting indexes #125

timgraham · 2024-09-11T21:46:48Z

WaVEV · 2024-10-06T08:39:28Z

django_mongodb/introspection.py

    def table_names(self, cursor=None, include_views=False):
        return sorted([x["name"] for x in self.connection.database.list_collections()])
+
+    def get_constraints(self, cursor, table_name):
+        indexes = self.connection.database[table_name].index_information()


Could we use self.connection.get_collection instead of .database[table_name]. But I don't know if we have to keep get_collection.

Yes, I'll rebase this after #152 and incorporate those changes.

Jibola

LGTM. Some comments getting clarity around constraints, but that's it.

Jibola · 2024-10-08T13:45:00Z

django_mongodb/schema.py

+        # Meta.index_together (RemovedInDjango51Warning)
+        for field_names in model._meta.index_together:
+            self._add_composed_index(model, field_names)


For users of Django 5.1, the below for loop should include the index_together elements, right?

Support for Meta.index_together is removed in Django 5.1, so these lines will be removed.

Ahhh, looked over the documentation and see that this was deprecated since 4.2

Jibola · 2024-10-08T14:42:32Z

django_mongodb/schema.py

+                self._remove_field_index(model, old_field)
+                self._add_field_index(model, new_field)


NIT: Add then remove. A dropped index has no further use, but adding one may need to be used quickly.

Is the scenario you imagine zero-downtime migrations? In that case, I think the developer would be advised to roll out schema changes in advance of the application logic that uses those fields, and thus we shouldn't worry about this sort of optimization.

Otherwise, a site should be in maintenance mode while deploying a new version that requires schema changes.

Ahhh, okay. That was the exact case I had been thinking of.

Thought of it as a small optimization but if that's not standard practice I'm fine leaving it as is.

Jibola · 2024-10-08T15:03:40Z

django_mongodb/schema.py

+        if index.contains_expressions:
+            return


Should we not raise a NotSupportedError or what's the tactic for handling indexes with expressions. UUIC these are complex types that exist beyond the standard ASC/DESC indexing capabilities of MongoDB.

They are ignored (in case the index is useful on another database backend) as per https://docs.djangoproject.com/en/dev/ref/models/indexes/#django.db.models.Index.expressions.

Jibola · 2024-10-08T15:42:37Z

django_mongodb/schema.py

+        if index.contains_expressions:
+            return
+        index_orders = (
+            [(field.column, 1)]


NIT: Could we have 1 and -1 refer to a constant like in introspection.py?

The code here checks out. I think it just assists with readability.

Sure. It's pymongo.ASCENDING / DESCENDING.

Yep. Those are the best enums to use.

Jibola · 2024-10-08T15:43:19Z

django_mongodb/schema.py

+            [(field.column, 1)]
+            if field
+            else [
+                (model._meta.get_field(field_name).column, 1 if order == "" else -1)


It's safe to assume this will either be an empty string or "-1"?

"" or "DESC" as per https://github.com/django/django/blob/22e21ec641c6c533d31bef0db876c53207d14920/django/db/models/indexes.py#L66. I'll add a comment.

Jibola · 2024-10-08T15:52:00Z

django_mongodb/schema.py

+        collection = self.connection.database[model._meta.db_table]
+        meta_index_names = {index.name for index in model._meta.indexes}
+        index_names = self._constraint_names(
+            model,
+            [field.column],
+            index=True,
+            # Retrieve only BTREE indexes since this is what's created with
+            # db_index=True.
+            type_=Index.suffix,
+            exclude=meta_index_names,
+        )


why do we need to use index_names. Is it because there will be special prefixes attached to the naming on django-based indexes?

The index name contains model._meta.db_table and if that changes after the index was created, the index isn't regenerated with a new name.

And also creating indexes in create_model() and add_field(), and removing them in remove_field().

timgraham marked this pull request as draft September 12, 2024 02:13

timgraham force-pushed the add-indexes branch from 3c25544 to fa74fae Compare September 13, 2024 00:40

timgraham changed the title ~~add partial support for creating indexes and unique constraints~~ add support for creating and deleting indexes Sep 13, 2024

timgraham marked this pull request as ready for review September 13, 2024 01:53

timgraham requested review from WaVEV and removed request for WaVEV September 13, 2024 01:53

timgraham marked this pull request as draft September 13, 2024 14:57

timgraham force-pushed the add-indexes branch from fa74fae to 945f7c8 Compare September 16, 2024 13:26

timgraham marked this pull request as ready for review September 16, 2024 14:53

timgraham marked this pull request as draft September 17, 2024 15:01

timgraham force-pushed the add-indexes branch 2 times, most recently from a0653e3 to 8520d2c Compare September 18, 2024 12:53

timgraham marked this pull request as ready for review September 18, 2024 12:56

timgraham marked this pull request as draft September 19, 2024 14:32

timgraham force-pushed the add-indexes branch 5 times, most recently from 2e3cbc3 to 4b0172c Compare September 30, 2024 12:23

timgraham force-pushed the add-indexes branch from 4b0172c to 9381229 Compare October 1, 2024 14:45

timgraham marked this pull request as ready for review October 1, 2024 17:16

timgraham force-pushed the add-indexes branch from 9381229 to 65c0476 Compare October 3, 2024 13:37

timgraham requested review from Jibola and WaVEV October 3, 2024 16:45

WaVEV reviewed Oct 6, 2024

View reviewed changes

WaVEV approved these changes Oct 6, 2024

View reviewed changes

Jibola approved these changes Oct 8, 2024

View reviewed changes

timgraham mentioned this pull request Oct 8, 2024

implement SchemaEditor.add/alter/remove_field() #115

Closed

timgraham force-pushed the add-indexes branch from 65c0476 to 8a98f17 Compare October 9, 2024 00:46

timgraham added 3 commits October 9, 2024 10:44

implement SchemaEditor.add/remove_index()

f6d48c2

And also creating indexes in create_model() and add_field(), and removing them in remove_field().

implement SchemaEditor.alter_index_together()

4fd0895

add index creation/deletion support to SchemaEditor.alter_field()

1c0706b

timgraham force-pushed the add-indexes branch from 8a98f17 to 1c0706b Compare October 9, 2024 14:46

timgraham merged commit 1c0706b into mongodb-labs:main Oct 9, 2024
4 checks passed

timgraham deleted the add-indexes branch October 9, 2024 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for creating and deleting indexes #125

add support for creating and deleting indexes #125

timgraham commented Sep 11, 2024

WaVEV Oct 6, 2024

timgraham Oct 7, 2024

Jibola left a comment

Jibola Oct 8, 2024

timgraham Oct 8, 2024

Jibola Oct 9, 2024 •

edited

Loading

Jibola Oct 8, 2024

timgraham Oct 8, 2024 •

edited

Loading

Jibola Oct 9, 2024

Jibola Oct 8, 2024

timgraham Oct 8, 2024

Jibola Oct 8, 2024

timgraham Oct 8, 2024

Jibola Oct 9, 2024

Jibola Oct 8, 2024

timgraham Oct 8, 2024

Jibola Oct 8, 2024

timgraham Oct 8, 2024

		self._remove_field_index(model, old_field)
		self._add_field_index(model, new_field)

add support for creating and deleting indexes #125

add support for creating and deleting indexes #125

Conversation

timgraham commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jibola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jibola Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jibola Oct 9, 2024 •

edited

Loading

timgraham Oct 8, 2024 •

edited

Loading