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

Add a draft for indexes. #6

Closed
wants to merge 2 commits into from
Closed

Add a draft for indexes. #6

wants to merge 2 commits into from

Conversation

mjtamlyn
Copy link
Member

Some draft API specifications for custom indexes. Key points:

  • Introduction of django.db.models.indexes
  • Support for custom classes to db_index
  • Introduction of Meta.indexes
  • spatial_index would be deprecated and a corresponding chunk of code removed from gis, allowing fields to specify their own index type, via Field.get_default_index

class Meta:
indexes = [
models.BTree(fields=['slug', 'name']),
models.FunctionalIndex(expression(F('length') * F('average_capacity')),
Copy link
Member

Choose a reason for hiding this comment

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

Is expression a callable here? Or simply the first arg of models.FunctionalIndex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops yeah, expression=

Copy link
Member

Choose a reason for hiding this comment

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

I expect FuntionalIndex to be somewhat complex to set up properly. You will need to have an query so that F() objects can be resolved. Then you need to extract the actual SQL of the expression somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't intend to try to write this before the changes to expressions. It also would ideally support transform='field__transform' or similar as well. This should be a little easier to extract the sql from I think.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, FunctionalIndex on transforms and custom expressions needs to be visited later on. It might be actually somewhat straightforward to do this, you'll need to mock the query, but that might turn out to be nothing more than mocking resolve_ref(). (see akaariai/django@04ee37a, the F<->query API is actually very clean in that proposal).

@akaariai
Copy link
Member

+1 to this proposal in general.

How does change detection work for migrations? When I add Meta.indexes = FooIndex('bar'), how does the data flow go between the models, indexes and migrations layers? My understanding is that the migrations framework will detect that the Meta.indexes has changed, then the models layer will tell the migrations that due to the change, a new index FooIndex is needed. Finally, the migrations framework implements this change (perhaps by calling as_sql()). Similarly when the FooIndex is removed, the migrations framework detects the change, the model layer tells that FooIndex needs to be deleted. Is this correct?

There isn't anything about how indexes are removed - are there cases where special SQL is needed, or is DROP INDEX index_name; sufficient always? If so, this should be easy to handle properly.

Changes in index definition should be easy - first the new index is created, then the old index is dropped.

Of course, we need a good way to name indexes. I think automatic naming should be the default, but users should be able to define index names if they so wish. Index names should never change - the index name is the "primary key" for migrations.

@mjtamlyn
Copy link
Member Author

Hmm, Meta.indexes can have indexes which can also be created with db_index. Perhaps the best solution like with index_together is to treat db_index as a shortcut for settings indexes and the migrations framework can deal with all non-unique migrations together. Obviously we need to be able to handle migrations written before these changes which would not have indexes in their deconstruction and would have db_index or index_together.

I think removing indexes would be handled the same way as creating them. For almost every index DROP INDEX name is sufficient, but there's extra fun with spatialite - it's actually a DROP TABLE statement (...WHAT?)

I think accepting name as a kwarg seems reasonable for fixing the name. If you use the shortcut db_index then perhaps you can't set the name as you are passing the class in so we can bind it to the field.

@mjtamlyn
Copy link
Member Author

Ooh one interesting point with name changes are if we are using automatic names what do we do when the field name changes? I'm not sure how this is handled at the moment but I imagine there could be issues with add field with index -> change field name -> remove index

models.BTree(fields=['slug', 'name']),
models.FunctionalIndex(expression=(F('length') * F('average_capacity')),
type=models.BTree),
]
Copy link
Member

Choose a reason for hiding this comment

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

I think indexes, and also unique_together, should be a set rather than a list; order has no meaning. And since we're no longer supporting Python 2.6, the syntax is even no longer bulkier -- just replace [] with {}.

@mjtamlyn
Copy link
Member Author

The start of a POC is available at mjtamlyn/django@django:master...mjtamlyn:indexes

At the moment I have simply moved the existing logic around, putting the results of index_together into a new Options.indexes, and making db_index be an Index() rather than True. Index() also generates its own sql for addition and removal, and its own name.

At present the naming logic is still maintained in SchemaEditor as well as in Index because it is also used for unique constraint names, and automatic foreign key indexes. It should be possible to just create the relevant Index() objects once we allow additional logic in them, such as uniqueness.

I believe the test suite passes.

Comments on the initial work would be very welcome!


``Index()`` subclasses will also have a ``supports(backend)`` method which
returns ``True`` or ``False`` depending on whether the backend supports that
index type.
Copy link
Member

Choose a reason for hiding this comment

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

We have the same issue here as with custom lookups and fields w.r.t 3rd-party backends. We should provide easy ways for both backends to specify index support and indexes to specify backend support.

@shaib
Copy link
Member

shaib commented Sep 21, 2014

Make your POC a PR so we can tell you things like "in django/db/migrations/operations/models.py
line 328 you have the from_state and to_state arguments reversed" :-)

@mjtamlyn
Copy link
Member Author

@shaib django/django#3253

@jacobian
Copy link
Member

@mjtamlyn can you update this to follow the newish format defined by DEP 1? If so, I'll merge (or you can feel free to do so yourself)

sqlite backend. As far as I can tell ``spatial_index`` is currently not handled
by any ``SchemaEditor`` for MySQL or Oracle, despite the old
``DatabaseCreation`` classes handling it correctly (and very similarly to
Postgis.

Choose a reason for hiding this comment

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

missing close bracket here.

@carljm
Copy link
Member

carljm commented Dec 1, 2014

It seems to me that autogenerating index names and then introspecting indexes by constituent fields will likely not be adequate with this level of index customizability. The instantiation API for Index objects probably should include a way to explicitly set the index name, and the index name should probably be tracked as part of the deconstructed index object.

More discussion at https://groups.google.com/d/msgid/django-developers/c1e8088e-37d3-45d9-9cda-1b8f965d9124%40googlegroups.com

@jacobian
Copy link
Member

At this point this is still missing a lot of the basic formatting and team required by the DEP process, so I'm going to close this PR. If and when a team emerges, please feel free to re-open or open a new PR with this DEP. Thanks!

@jacobian jacobian closed this Apr 14, 2015
@auvipy
Copy link

auvipy commented Oct 28, 2015

possibilities for class based index's in next django release? probably 1.10?

@jdufresne
Copy link
Member

In PR django/django#5766 for trac ticket #20581, @charettes mentioned that this DEP could be a possible solution for introducing a mechanism allowing for deferrable unique constraints. Are there plans to address this as part of this proposal? I see this as an additional motivation for richer index/constraint syntax.

@mjtamlyn
Copy link
Member Author

mjtamlyn commented Dec 4, 2015

I don't see any reason why this couldn't be supported by a more powerful indexes/constraints API.

@timgraham timgraham deleted the indexes branch November 29, 2018 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants