On 2021-02-27 13:31, Nicolas Évrard wrote: > >On 2021-02-27 11:12, Nicolas Évrard wrote: > >> >> I have made the proposal in the review which is kind of a mix of > >> >> all the ideas in this issue. > >> >> > >> >> - Definition of the index in a specific attribute in \_\_setup\_\_ > >> > > >> >I do not think it is correct to store a backend specific object on > >> >the Model. > >> > >> I don't see a problem with that. > >> It's not like we could change from one backend to another easily. > > > >It just makes the object unpractical. > >The Model definition can not depend on the backend. > > From my point of view the Model definition does not depend on the > backend even with this addition. It is as soon as you store on it an object that are different depending on the backend. > Because it is only when the definition is realized (ie executed), that > the index it contains turn into their implementations. No because the class which store the index object is different per backend. It is not the execution that is different. > >It must be the same no matter the backend because it is an > >abstraction. Otherwise a model is linked to a specific backend. > > The model is linked to the specific backend only when the code is > executed. It's a transient state. It is not when the code is executed, it is directly at the __setup__. So yes it is a transient state on the model and this is precisely why it is a bad design. Model must be static. > >> >Indeed by looking at syntax of both backends they share almost the > >> >same syntax/parameters. > >> > >> SQLite is like the minimum of index creation. > >> I also looked at MariaDB and Oracle's version of CREATE INDEX (that's > >> why 'WHERE' is in the kwargs instead of the arguments). The similarity > >> of the syntax is kind of an artefact of SQLite simplicity. > > > >There is no point to have arguments that depend on the backend. They can > >never be used when defining a Model because it does not depend on the > >backend. > > In a perfect world there would be no index information in the models > and this work would be left to DBAs. I do not agree with this statement. The DBA is the last resort but developer create queries that must be optimized by indexes. So it is the developer who knows what index must be created. The DBA is there for the unattended queries. > So since we are compromising with the purity of the model we might as > well make this explicit. I do not want to compromise on purity. Model must never depend directly on the backend. > >> >So I think we could have an object in modelsql.py like the Constraint > >> >that store the index definition to be passed to the table handler. > >> > >> First I think that the TableHandler class is already way too big. It's > >> why I chose to make another class. > > > >I do not think it simplify anything to move two methods from one class > >to another in the same file. > > At least it's a start. Having those backend information structured > instead of having everything packed in the same class seems like a > good idea. I see no improvement there, just backward compatibility breakage. Also with this separation we loose the optimisation that prevent to run index creation query if the index already exist. Of course there is the "IF NOT EXISTS" but it is an extra query. (Now we could argue that retrieving the indexes can be complex). But as index is indeed a manipulation of the table (index can not be cross-table), it is logical that the table handler handles them. > >But I really think it is not good to have to define the method to use > >but we must define which properties we need for the index and the > >backend should choose the type that works the best. > > I don't think that creating another abstraction will help. It will > just add another indirection and the mapping between our "langage" and > the different indexes implementation will be awkward. So with such reasoning let's just throw away the ORM, it is just an indirection to SQL. This is precisely the added value of the framework to provide a good abstract to such thing. > Moreover it will add another point where beginners will have to learn > how we do stuffs and make the learning curve steeper. I do not see why a beginner will know better the specific type of indexes of PostgreSQL and in which case which one should be used. The framework must add value! > >> >I would say that only the method is a specific that is good to > >> >support but it is mainly related to the kind of operation for which > >> >the index it created. > >> > >> I don't understand. Are you saying that you don't think the people > >> should specify the method but they should rather define the operation? > > > >Indeed, I can see this kind of operations: > > > > - equality > > - range (<, >, etc) > > - begin pattern > > - 2d or vector > > - multi (inverted) > > > >For postgresql it would result in: > > > > ['equality'] -> HASH > > ['equality', 'range'] -> BTREE > > ['begin pattern'] -> BTREE > > ['vector'] -> GIST > > ['multi'] -> GIN > > I think this is trying to be more clever than what the feature > requires. This is the added value of the framework. Probably the naming and definition must be refined. We could also have good default value depending of the type column used. > >Now if we have ['range', 'vector'], this may be difficult to choose. > >We have three options: > > > > - create no index > > - create multi index for each group of cases > > - create one index as best effort > > > >I think we should do the last one, just find the type of index that > >provide the most of operations (+ internal preferences for equality > >ex: we prefer BTREE over HASH). > > This is precisely the kind of difficulty I want to avoid so that we > don't have to make a guess. If we let the people decide according to > their knowledge of their Model than this question is not there at all. Again this is the added value otherwise we just let developer create the indexes themselves.
|2021-02-27 14:32:03||ced||set||recipients: + yangoon, nicoe, albertca, pokoli, jcavallo, reviewbot|
|2021-02-27 14:32:03||ced||link||issue5757 messages|
Showing 10 items. Show all history (warning: this could be VERY long)