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

Fixed #26709 -- Add class based indexes and CreateIndex operation #6726

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@akki
Contributor

akki commented Jun 5, 2016

Ticket #26709.

tablespace_sql = " " + tablespace_sql
if not hasattr(self, 'name'):
self.name = schema._create_index_name(self.model, columns, suffix=suffix)

This comment has been minimized.

@akki

akki Jun 6, 2016

Contributor

I am not sure if this is the right place to do this. Please have a look.

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 6, 2016

Member

I do think this would be an obvious extension point for a subclass. Perhaps get_name()?

This comment has been minimized.

@akki

akki Jun 7, 2016

Contributor

Should I write a new method get_name for Index class, which would create a name for the index independent of the schema. That way we will also have a uniform naming standard for the indexes, no matter which database the user uses.
But we might have to think about backward compatibility for this.

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

I think the name should be generated as part of the index class and should be database backend independent, otherwise your migrations would yield different names depending on the database used to generate the migrations.

.. class:: DopIndex(model_name, name)
Deletes the provided index from it's model.

This comment has been minimized.

@mjtamlyn
"""
project_state = self.set_up_test_model("test_crin")
Pony = project_state.apps.get_model("test_crin", "Pony")
index = models.Index("pink", model=Pony)

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 6, 2016

Member

You shouldn't need to set the model here? You wouldn't have access to it in the migration file anyway. I think state_forwards should set up the connection for you

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

Should it be removed as an argument of the Index then?

This comment has been minimized.

@akki

akki Jun 20, 2016

Contributor

We use it in get_name.

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

Sorry, I meant: should model be removed as an argument of Index.__init__() if it can't be used as an argument in normal usage?

def __init__(self, model_name, name):
self.model_name = model_name
self.name = name

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 6, 2016

Member

Why does DropIndex take name, but CreateIndex takes an Index? That seems a little counterintuitive.

This comment has been minimized.

@timgraham

timgraham Jun 15, 2016

Member

It parallel's AddField/RemoveField (the latter takes only the name).

This comment has been minimized.

@timgraham

timgraham Jun 16, 2016

Member

Although using only the name might cause usability issues in the case of an auto-generated name. The user will have to inspect the database or something in order to determine the index name if they're writing the migration by hand.

This comment has been minimized.

@akki

akki Jun 16, 2016

Contributor

These are @MarkusH 's thoughts when I raised the same concern earlier today:

If one writes his own indexes he should really give them explicit names.
It's on us to document that you should ALWAYS define the name.

Right now the user would have to use sqlmigrate or inspect the db in order to determine the auto-generated index name.

This comment has been minimized.

@akki

akki Jun 16, 2016

Contributor

And once adding indexes is supported through the Meta class, I am not sure if there would be many users adding indexes from migrations and falling into this situation.

This comment has been minimized.

@MarkusH

MarkusH Jun 19, 2016

Member

I briefly remember that there was an issue with wrong index names a while ago, that somehow got in people's projects. I can't recall the details, but all migration operations should enforce a deterministic index name present. That is, AddIndex checking for self.index.name and RemoveIndex checking for self.name.

This comment has been minimized.

@charettes

charettes Jun 20, 2016

Member

Just thinking out loud here, could we add a notion of index name and index db_name where the later would be database specific and based on db_table, db_column and nameand type?

Fields using db_index=True would generate Index(name='field_name') and we would require indexes added to _meta.indexes to have an explicit name?

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

I'm not following exactly what the idea is and what problem it's solving.

This comment has been minimized.

@charettes

charettes Jun 21, 2016

Member

It's solving the issue about DropIndex taking a name mentioned above and should make a lot stuff easier to deal with (such as index renames) as indexes will be identifiable by a user defined string.

Either all Index should have a user defined name like field or we should not try to mimic the RemoveField API and make DropIndex take an Index instead of a system generated name.

This comment has been minimized.

@akki

akki Jun 21, 2016

Contributor

Although using only the name might cause usability issues in the case of an auto-generated name. The user will have to inspect the database or something in order to determine the index name if they're writing the migration by hand.

Is this the problem you are suggesting to solve with this approach ?
Since this is only a problem when migrations are added by hand, I would prefer @MarkusH's "It's on us to document that you should ALWAYS define the name." approach. If we are to ask the users to explicitly give the index a name why give the pain to all the users, just give it to those who want to add indexes the non-traditional way (via AddIndex migration).
Also the statement "If one writes his own indexes he should really give them explicit names." makes a lot of sense to me. If the user needs to an index using AddIndex, I am quite sure he has (or can think of) a name for it.

Also it might be a bit tricky to handle the uniqueness of the user provided name.

model_state = state.models[app_label, self.model_name.lower()]
i, idx = model_state.get_index_by_name(self.name)
idx_list = model_state.options['indexes']
model_state.options['indexes'] = idx_list[:i] + idx_list[(i + 1):]

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 6, 2016

Member

I think you want options['indexes'].remove(idx_name) here. That would simplify this code, and I think together with the change in signature mentioned above, remove the need for get_index_by_name as well.

List.remove

``DropIndex``
---------------
.. class:: DopIndex(model_name, name)

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 6, 2016

Member

DropIndex

if 'name' in kwargs:
self.name = kwargs['name']
def as_sql(self, schema, suffix=''):

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 6, 2016

Member

Thinking about it, I'm not sure as_sql is the right name, given drop_sql. Perhaps create_sql would be better?

quote_name = schema.quote_name
return schema.sql_delete_index % {
"table": quote_name(self.model._meta.db_table),
"name": quote_name(self.name),

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 6, 2016

Member

You can't assume the presence of self.name here

This comment has been minimized.

@akki

akki Jun 7, 2016

Contributor

self.name is being set in create_sql in any case (here). So I guess if an index exists in the database, we can safely assume that self.name is set. Please let me know your thoughts on this.

@mjtamlyn

This comment has been minimized.

Member

mjtamlyn commented Jun 6, 2016

I think this is broadly heading in the right direction. I'd like to see some example migration file code. Perhaps it's worth you setting up a simple project that we can build up the functionality but makes it easy to show that it works.

I think Index.as_sql is likely to need some refactoring as we make it more complex to support other options. In particular, I think that just using interpolation into schema.sql_create_index may prove too simplistic.

@timgraham

This comment has been minimized.

Member

timgraham commented Jun 8, 2016

About naming, we have:
CreateModel, DeleteModel
AddField, RemoveField,

It would be nice if the new operations were consistent with these names. I guess I'd go with AddIndex/RemoveIndex since, like fields, these are ancillary things to models -- other opinions?

.. versionadded:: 1.11
Creates a new index in the database table for the given model. ``model_name`` is the model's name

This comment has been minimized.

@timgraham

timgraham Jun 8, 2016

Member

please wrap documentation lines at 79 characters

.. class:: Index(*fields, **options)
.. versionadded:: 1.11

This comment has been minimized.

@timgraham

timgraham Jun 8, 2016

Member

You can put this annotation below the heading at the top of the document rather than repeating it everywhere.

Built-in indexes
================
.. note::

This comment has been minimized.

@timgraham

timgraham Jun 8, 2016

Member

I don't think we need to repeat this -- it's already mentioned earlier.

.. attribute:: BaseIndex.fields
Index classes take the field names fo which they are being used as arguments

This comment has been minimized.

@timgraham

timgraham Jun 8, 2016

Member

from the docs build error: "fo"

.. attribute:: BaseIndex.fields
Index classes take the field names fo which they are being used as arguments
directly during initialisation.

This comment has been minimized.

@timgraham

timgraham Jun 8, 2016

Member

from the docs build error: "initialisation" -> "initialization"

Index classes define a specific database index which can be created through
them. This document explains the API references of :class:`BaseIndex` which is
the base class for all other index classes. This includes the `index options`_
and `built-in indexes`_ Django offers. For now the indexes can be created in

This comment has been minimized.

@timgraham

timgraham Jun 8, 2016

Member

chop "For now"?

class CreateIndex(IndexOperation):
"""
Creates an index for a model.

This comment has been minimized.

@timgraham

timgraham Jun 8, 2016

Member

Use PEP257 verb style for new docstrings: "Create", "Delete", "Return", etc..

@charettes

This comment has been minimized.

Member

charettes commented Jun 8, 2016

About naming, we have:
CreateModel, DeleteModel
AddField, RemoveField,

It would be nice if the new operations were consistent with these names. I guess I'd go with AddIndex/RemoveIndex since, like fields, these are ancillary things to models -- other opinions?

Agreed.

@MarkusH

This comment has been minimized.

Member

MarkusH commented Jun 8, 2016

About naming, we have:
CreateModel, DeleteModel
AddField, RemoveField,

It would be nice if the new operations were consistent with these names. I guess I'd go with AddIndex/RemoveIndex since, like fields, these are ancillary things to models -- other opinions?

Agreed.

The names are derived from the SQL syntax which is CREATE INDEX and DROP INDEX. (CREATE TABLE / DROP TABLE, ADD COLUMN / DROP COLUMN [I agree the latter is poorly chosen]).

model_state = state.models[app_label, self.model_name.lower()]
if not hasattr(self.index, 'model'):
self.index.model = state.apps.get_model(app_label, self.model_name)
if 'indexes' not in model_state.options:

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

This should never be the case. ModelState should have a default indexes = []

self.index.model = state.apps.get_model(app_label, self.model_name)
if 'indexes' not in model_state.options:
model_state.options['indexes'] = []
model_state.options['indexes'].append(self.index)

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

Does the ordering of indexes on a database matter? I'm not talking about the order of fields an index applies to, but the indexes itself.

This comment has been minimized.

@akki

akki Jun 10, 2016

Contributor

The indexes have no ordering in the database, they are all independent and individual entities AFAIK.

def state_forwards(self, app_label, state):
model_state = state.models[app_label, self.model_name.lower()]
model = state.apps.get_model(app_label, model_state.name)

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

In CreateIndex you check for hasattr(self.index, model). Any reason you're not doing it here?

)
def describe(self):
return "Create index on field(s) %s of model %s" % (

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

Add the index name?

This comment has been minimized.

@akki

akki Jun 9, 2016

Contributor

The index name is not generated with the initialization of an instance of Index (because the current implementation in Django makes it database backend dependent) unlike the describe method which should be available as soon as an instance is created.

@MarkusH

This comment has been minimized.

Member

MarkusH commented Jun 8, 2016

About naming, we have:
CreateModel, DeleteModel
AddField, RemoveField,

It would be nice if the new operations were consistent with these names. I guess I'd go with AddIndex/RemoveIndex since, like fields, these are ancillary things to models -- other opinions?

Agreed.

The names are derived from the SQL syntax which is CREATE INDEX and DROP INDEX. (CREATE TABLE / DROP TABLE, ADD COLUMN / DROP COLUMN [I agree the latter is poorly chosen]).

Well there we go, I don't even recall the names of the operations: it's CreateModel/DeleteModel and AddField/RemoveField. Anyhow, I find CreateIndex/DeleteIndex more intuitive (the keyword for the creating a new index is still the same as in SQL)

@charettes

This comment has been minimized.

Member

charettes commented Jun 8, 2016

As indexes are related to a table/model I find AddIndex/RemoveIndex more appropriate.

Is there a reason not to use a single AlterIndexes operation analogous to AlterUniqueTogether/AlterIndexTogether?

@@ -17,4 +17,5 @@ Model API reference. For introductory material, see :doc:`/topics/db/models`.
lookups
expressions
conditional-expressions
indexes

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

I'd probably add this right after fields

operations = [
migrations.CreateIndex(
model_name='Author',
models.Index('name', 'age', name='my_index_name') # An index on the name and age fields

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

2 spaces before #, please.

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

I'd also replace 'name' with 'title' to prevent confusion that 'name' is a column and not the name of the index.

Technically, indexes are defined in `django.db.models.indexes`, but for
convenience they're imported into :mod:`django.db.models`; the standard
convention is to use ``from django.db import models`` and refer to the indexes
as ``models.<Index-name>``.

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

I'd make it modes.<IndexClass> to highlight they are classes.

if 'model' in kwargs:
self.model = kwargs['model']
if 'name' in kwargs:
self.name = kwargs['name']

This comment has been minimized.

@MarkusH

MarkusH Jun 8, 2016

Member

I'm wondering if it makes sense to have self._name default to None and a property name that either returns self._name or automatically generates one and stores it on self._name which is then returned.

With the current implementation the following won't easily be possible to deconstruct for migrations:

class MyModel(models.Model):
    class Meta:
        indexes = [Index('id')]

because indexes[0] doesn't have a name which we always want to add to the deconstructed index class to ensure we can change the way index names are generated and not break other people's code.

This comment has been minimized.

@akki

akki Jun 10, 2016

Contributor

How can we ensure that we don't break other people's code even with this strategy. You are referring to code that has already been written by developers to create indexes (using index_together, db_index, etc.), am I right ?

# The type of index - BTree, Hash, RTree, etc.
# Used in deconstruction to differentiate indexes of different types with
# the same set of fields.
index_type = None

This comment has been minimized.

@charettes

charettes Jun 8, 2016

Member

Can't you rely of self.__class__ for this case?

This comment has been minimized.

@akki

akki Jun 10, 2016

Contributor

It will also be used in the describe method in the Operations and other places afterwards (maybe like suffix in index name). I'll keep in mind to see if all that could be done without introducing index_type.

@@ -348,6 +348,18 @@ def alter_index_together(self, model, old_index_together, new_index_together):
fields = [model._meta.get_field(field) for field in field_names]
self.execute(self._create_index_sql(model, fields, suffix="_idx"))
def add_index(self, index):

This comment has been minimized.

@timgraham

timgraham Jun 16, 2016

Member

I'd move these methods above "def alter_unique_together" so those alter methods aren't separated from their helper, _delete_composed_index.

@timgraham

This comment has been minimized.

Member

timgraham commented Jun 16, 2016

If you're waiting for reviews, maybe you want to look at https://code.djangoproject.com/ticket/24442 in the meantime.

@akki

This comment has been minimized.

Contributor

akki commented Jun 19, 2016

I have updated the algorithm.
Two indexes are identical if they are on the same table, have the same set of fields and are of the same type, so I have taken all three of these into account while hashing.
Also, I have alloted a share to each of them in the index name to keep it meaningful (as much as possible). I have given more share to table-name than field-name because table-name contains both app label and model name which usually makes it significantly larger than the column name.

if index_name[0] == "_":
index_name = index_name[1:]
# It can't start with a number on Oracle, so prepend D if we need to
if index_name[0].isdigit():

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

Add validation that user-provided names don't start with a digit?

index_type = 'idx'
def __init__(self, *fields, **kwargs):

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

Why **kwargs instead of name=None, model=None? By the way, the documentation says "options" instead of "kwargs".

This comment has been minimized.

@akki

akki Jun 20, 2016

Contributor

We might add type keyword argument later if other index types prove out to be simple enough that they don't need a separate class of their own. This is something we have deferred to subsequent work.

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

I don't think that just because you might add more kwargs later doesn't mean you should use kwargs instead of declaring the options you have now. Am I missing the problem?

This comment has been minimized.

@charettes

charettes Jun 20, 2016

Member

I don't think we should hijack *args for fields, a fields kwarg makes more sense to me.

model should be assigned by Options just like Field.model is.

What do you think of the name, db_name approach suggested above? In this case the signature should look like (name, fields, db_name=None, **options) and there would be no need to validate name.

This comment has been minimized.

@akki

akki Jun 21, 2016

Contributor

I don't think we should hijack *args for fields, a fields kwarg makes more sense to me.

Why is that a hijack ? The argument of an Index class is it's fields. I used it mainly because defining Index('foo') seems syntactically much cleaner than Index(fields=['foo']) to me. Am I missing something here ? I agree on replacing **kwargs with name=None though.

model should be assigned by Options just like Field.model is.

This is probably more suited once we introduce indexes to Options, let's keep that for the next checkpoint.

What do you think of the name, db_name approach suggested above? In this case the signature should look like (name, fields, db_name=None, **options) and there would be no need to validate name.

db_name still needs to validated if not name. I have left some other comments regarding the approach here.

"Index names cannot be longer than %s chars." % MAX_NAME_LENGTH
)
if 'model' in kwargs:
self.model = kwargs['model']

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

Any problem with allowing self.model = None. I think conditional attributes which require hasattr isn't the best design.

Test the AddIndex operation.
"""
project_state = self.set_up_test_model("test_adin")
index = models.Index("pink")

This comment has been minimized.

@timgraham

timgraham Jun 20, 2016

Member

Why don't you provide the model here? Why is model an optional argument?

This comment has been minimized.

@akki
@timgraham

This comment has been minimized.

Member

timgraham commented Jun 20, 2016

Some cosmetic edits at akki#2.

@akki

This comment has been minimized.

Contributor

akki commented Jun 22, 2016

@timgraham Added tests for index names starting with numbers and underscores (regarding get_name) if that's what you meant.

def state_forwards(self, app_label, state):
model_state = state.models[app_label, self.model_name.lower()]
if not hasattr(self.index, 'model'):

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

I think this if statement can be removed as it doesn't seem to ever evaluate to False when running the test suite.

This comment has been minimized.

@akki

akki Jun 22, 2016

Contributor

It does for new Index instances.

This comment has been minimized.

@akki

akki Jun 22, 2016

Contributor

Oops, I forgot the not. Done.

self.normalise_name()
return self._name
def normalise_name(self):

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

I was expecting to raise a ValueError if these conditions aren't met, similar to what we do with "Index names cannot be longer". I don't think it's a good idea to modify the user provided value as it seems like that would only cause confusion.

def deconstruct(self):
"""
Return a 3-tuple of class import path, positional arguments, and

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

I think we can drop this docstring. We don't need to repeat it for every object that implements deconstruct().

def test_deconstruction(self):
index = models.Index(fields=['title'])
path, args, kwargs = index.deconstruct()
self.assertEqual(path, "django.db.models.Index")

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

Use single quotes consistently.

column_names = self.fields
hash_data = [table_name] + column_names + [self.index_type]
index_unique_hash = self._hash_generator(*hash_data)
table_part = table_name.replace('"', '').replace('.', '_')

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

The purpose of these replacement lines isn't obvious. Could you add a comment about them? Also no tests fail if they're removed.

This comment has been minimized.

@akki

akki Jun 22, 2016

Contributor

Sorry but even I am not sure what these replacements are for. They were introduced here and I have been unable to find any tests that are/were related to them. Also, removing them causes issues during migrate when I had a db_table='name"with' in my Meta with the error

Django.db.utils.OperationalError: near "with": syntax error

This comment has been minimized.

@timgraham

This comment has been minimized.

@akki

akki Jun 24, 2016

Contributor

Thanks. Done.

.. versionadded:: 1.11
Removes the provided index from the given model.

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

Please add a description of the arguments as done for AddIndex.

``Index`` options
=================
.. class:: Index(fields, name)

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

This doesn't reflect the latest signature.

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

I guess we shouldn't have .. class:: Index twice though (it also appears under "Built-in" indexes) since that will be ambiguous from a linking perspective.

.. class:: Index(fields, name)
``*fields``

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

remove *

The name of the index. If ``name`` isn't provided Django will auto-generate a
name. For compatibility with different databases, index names cannot be longer
than 30 characters and should not start with a number(0-9) or underscore(_).

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

add a space before parentheses: "number (0-9) or underscore (_)"

``Index``
---------
.. class:: Index(fields, name=None)

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

fields=[]

# To check if auto-generated name is not more than 30 characters
index = models.Index(fields=['title'])
index.model = Book
self.assertTrue(len(index.name) <= 30)

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

Since index name generation is deterministic, I don't think this is needed. test_name_auto_generation already checks for a specific name. If this index is truncated is some different way, then it would be okay to keep it but check for the actual name rather than testing the length.

def create_sql(self, schema_editor):
columns = [field for field in self.fields]
fields = [self.model._meta.get_field(field) for field in self.fields]
if len(fields) == 1 and fields[0].db_tablespace:

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

I guess none of these lines aren't tested since I don't see any new tests that use db_tablespace.

if self._name:
self.normalise_name()
if len(self._name) > MAX_NAME_LENGTH:
raise ValueError("Index names cannot be longer than %s characters." % MAX_NAME_LENGTH)

This comment has been minimized.

@timgraham

timgraham Jun 22, 2016

Member

Use single quotes

if errors:
raise ValueError(errors)
@property

This comment has been minimized.

@timgraham

timgraham Jun 23, 2016

Member

I think this could be @cached_property so it doesn't have to be calculated on every access.

table_part = table_part[:11]
if len(field_part) > 7:
field_part = field_part[:7]
if len(hash_part) > 10:

This comment has been minimized.

@timgraham

timgraham Jun 23, 2016

Member

The hexdigest will always be a fixed length so this only happens if the provided suffix is too long, correct? In that case, I think it would be better to raise an error that the provided suffix is too long.

This comment has been minimized.

@akki

akki Jun 23, 2016

Contributor

In fact, suffixes aren't something input by users. They are something for us to decide. I wonder if there's a better way to keep future contributors informed about this restriction than raising an error.
While we are at this, do we really need customisation of suffixes. My idea is to keep different suffixes for indexes of different type like _rt for rtree, _hsh for hash, etc. which is exactly what I am planning to store in index_type in the future, so get_name doesn't need to have the suffix argument anymore.

This comment has been minimized.

@timgraham

timgraham Jun 24, 2016

Member

It sounds like maybe index_type could be used as the suffix and this method doesn't need that argument. I guess the question is whether index_type should be limited to 3 characters or if truncating the first 3 characters of "index_type" as the suffix is okay.

This comment has been minimized.

@akki

akki Jun 24, 2016

Contributor

For now, I cannot think of any other place I'll be using index_type. So maybe it's just best to call this variable suffix instead of index_type. And it is fair to limit suffix to 3 chars.

This comment has been minimized.

@akki

akki Jun 24, 2016

Contributor

But I still am not sure if raising an error is the best way to inform future developers to keep suffix not longer than 3 chars.

This comment has been minimized.

@timgraham

timgraham Jun 24, 2016

Member

We could add assert len(index_name) <= 30, Index too long for multiple database support. Is self.index_type longer than 3 characters?

This comment has been minimized.

@akki

akki Jun 24, 2016

Contributor

Ahh!
Done all changes.

@@ -201,7 +201,9 @@ is set, its column name).
Creates a new index in the database table for the given model. ``model_name``
is the model's name and ``index`` is an instance of the
:class:`~django.db.models.Index` class.
:class:`~django.db.models.Index` class. It is highly recommended to pass a name

This comment has been minimized.

@timgraham

timgraham Jun 23, 2016

Member

Please explain why it's recommended.

Fixed #26709 -- Introduced class based indexes
Added corresponding migration operations - AddIndex, RemoveIndex to use them

Thanks to markush, mjtamlyn, timgraham and charettes for reviewing and all the discussions.
@akki

This comment has been minimized.

Contributor

akki commented Jun 27, 2016

Updated PR addressing all comments. Rebased against master and squashed all commits.

@timgraham

This comment has been minimized.

Member

timgraham commented Jun 27, 2016

merged in 156e2d5, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment