Experimental index operators support#257
Conversation
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
@quanterion, congrats, this looks quite good - and it's great to receive a contribution which models PostgreSQL-specific features.
I've made a few comments to make this mergeable, take a look. In addition:
- Please add tests which demonstrate this. See how IndexMethod is currently tested.
- Add a documentation note under doc.
- You could also look at scaffolding support, i.e. including the index operator annotations included in models generated with EF Core. This really is extra-credit and not absolutely required (although it's fun!).
| public const string HiLoSequenceName = Prefix + "HiLoSequenceName"; | ||
| public const string HiLoSequenceSchema = Prefix + "HiLoSequenceSchema"; | ||
| public const string IndexMethod = Prefix + "IndexMethod"; | ||
| public const string IndexOperators = Prefix + "IndexOperator"; |
| protected virtual string[] GetOperators() { | ||
| var value = (string)Annotations.Metadata[NpgsqlAnnotationNames.IndexOperators]; | ||
| if (value != null) { | ||
| return value.Split(' '); |
There was a problem hiding this comment.
For correctness, it would be good to surround each operator class with quotes and delimit (i.e. for the theoretical case where an operator class contains a space...). Take a look at how the metadata for PostgreSQL extensions is implemented.
There was a problem hiding this comment.
https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/blob/dev/src/EFCore.PG/Metadata/PostgresExtension.cs#L158 Do you mean those functions? Should I extract them somewhere to avoid duplication?
There was a problem hiding this comment.
Yeah... I wouldn't really bother with extracting them to avoid duplication though.
There was a problem hiding this comment.
i assume delimit happens during set.
| var columnList = new List<string>(); | ||
| for (var k = 0; k < operation.Columns.Count - 1; ++k) { | ||
| var columnWithOperator = ColumnList(operation.Columns[k]); | ||
| if (k < operatorList.Count) { |
There was a problem hiding this comment.
It should also be possible to have a null in the operator list to skip explicitly defining the operator class for the correspond column (leaving it at the default).
| } | ||
| columnList.Add(columnWithOperator); | ||
| } | ||
| builder |
There was a problem hiding this comment.
This code is duplicated above, for the case where no operators are defined. Set up your column list (separately for each case), then render once.
There was a problem hiding this comment.
I can see in EF sources that ColumnList function transforms each column name and join them together. But I can't find a public function that transform each column name individually. That is why I use ColumnList to transform each column name in second case ( and that is not the optimal way) that is why I duplicated the code. What can you recommend me?
There was a problem hiding this comment.
I was just thinking about generating the contents inside the parentheses differently (based on the condition), and then having common code which appends the parentheses and the contents (nothing fancy).
|
@quanterion Do you plan to complete the feature? |
|
|
||
| protected virtual bool SetOperators(string[] operators) { | ||
| if (operators != null && operators.Count > 0) { | ||
| Annotations.SetAnnotation(NpgsqlAnnotationNames.IndexMethod, operators.Join(' ')); |
There was a problem hiding this comment.
Since an annotation can contain string array, there shouldn't be need to do a join here. Else it is causing inconsistency in the code that is the annotation value is string or string[]
roji
force-pushed
the
dev
branch
3 times, most recently
from
f320396 to
dfdc336
Compare
|
@quanterion am going to close this PR as there hasn't been any activity in a long time. If you'd like to pick this up again please go ahead (and let us know), in the meantime #481 was opened to track this feature. |
4 participants
Out of curiosity investigate a way to add index operators support.
It you would accept this functionality let me know how to work on this PR.
It could help to avoid fallback to raw SQL code migrations in cases like that #256