Add support for index operators by khellang · Pull Request #662 · npgsql/efcore.pg

khellang · 2018-10-09T10:48:29Z

Just picking up the good work done by @quanterion in #257.

I tried looking at scaffolding support as well, but it was a bit involved, so figured I'd punt on it for now and focus on getting the runtime support in.

Closes#481

austindrenski

Looks interesting, thanks for this! Just a few nits here.

austindrenski · 2018-10-09T11:34:39Z

+ string IndexColumnList(string[] columns, string[] operators)
+ {
+ if (operators == null || operators.Length == 0)
+ {


austindrenski · 2018-10-09T11:35:09Z

+ var @operator = operators[i];
+
+ if (string.IsNullOrEmpty(@operator))
+ {


austindrenski · 2018-10-09T11:35:33Z

+ var identifier = Dependencies.SqlGenerationHelper.DelimitIdentifier(v);
+
+ if (i >= operators.Length)
+ {


khellang · 2018-10-09T12:20:03Z

Looks like the CI failure is unrelated?

austindrenski · 2018-10-09T12:23:07Z

I think so, just started a rebuild on AppVeyor.

khellang · 2018-10-09T12:37:32Z

Indeed 🎉

austindrenski

Just took another look, and it looks good so far. Two additional considerations:

Would it be much more work to add support for sort options?
Could you take a look at how the current implementation handles a schema-qualified operator class?
- This may not be an issue here (or already be handled)...but I want to make sure we've covered the bases, given the recent work on schema-qualified types (#605, #626).

khellang · 2018-10-10T18:12:18Z

1. Would it be much more work to add support for sort options?

I don't know. It would definitely add to the complexity. If it's OK with you, I'd like to punt on it for now and maybe take a look at them later. I really need operator support, whereas I have never used or seen sort options being used before.

2. Could you take a look at how the current implementation handles a schema-qualified operator class?

You mean if someone pass myschema.blah_ops?

austindrenski · 2018-10-11T05:12:35Z

I don't know. It would definitely add to the complexity. If it's OK with you, I'd like to punt on it for now and maybe take a look at them later. I really need operator support, whereas I have never used or seen sort options being used before.

OK that sounds good—just wanted to make sure it wasn't an unintentional punt.

You mean if someone pass myschema.blah_ops?

Yes. For example, in the enum case, myschema.blah_ops is/was being delimited as "myschema.blah_ops" instead of "myschema"."blah_ops".

khellang · 2018-10-11T06:49:58Z

For example, in the enum case, myschema.blah_ops is/was being delimited as "myschema.blah_ops" instead of "myschema"."blah_ops".

Ah, yeah, that's probably happening here as well, unless SqlGenerationHelper.DelimitIdentifier handles schema-qualified identifiers? I'll take a look.

khellang · 2018-10-11T07:10:25Z

OK, I think the schema-qualified case should be handled now 👍

austindrenski · 2018-10-11T12:35:40Z

That looks great! Thanks for making those changes and the additional unit test.

@roji @YohDeadfall Could one of you give this a second review?

austindrenski

A few more questions regarding string[]/IReadOnlyList<string>.

austindrenski · 2018-10-19T03:33:36Z

@roji Did you want to give this a look before the merge?

austindrenski · 2018-11-12T20:08:15Z

@khellang Would you mind rebasing?

khellang · 2018-11-13T14:34:12Z

@austindrenski Done! ✨

austindrenski · 2018-11-13T14:44:01Z

@khellang Thanks!

@roji Do you have time today to give this a quick review?

roji

Looks great, thanks for the quality work and the reviewing!

@austindrenski you can go ahead and merge.

See npgsql#662, npgsql#704

See #662, #704

See npgsql#662, npgsql#704

khellang force-pushed the index-operators branch from 78c8d65 to 5657211Compare October 9, 2018 10:50

austindrenski suggested changes Oct 9, 2018
View reviewed changes

austindrenski added the enhancement label Oct 9, 2018

khellang force-pushed the index-operators branch from 5657211 to fa5d0dcCompare October 9, 2018 11:47

austindrenski approved these changes Oct 10, 2018
View reviewed changes

austindrenski suggested changes Oct 10, 2018
View reviewed changes

austindrenski approved these changes Oct 11, 2018
View reviewed changes

YohDeadfall reviewed Oct 11, 2018
View reviewed changes

Comment threadsrc/EFCore.PG/Metadata/INpgsqlIndexAnnotations.cs Outdated

YohDeadfall approved these changes Oct 12, 2018
View reviewed changes

Comment threadsrc/EFCore.PG/Metadata/NpgsqlIndexAnnotations.cs
Comment threadsrc/EFCore.PG/Utilities/Check.cs

austindrenski suggested changes Oct 12, 2018
View reviewed changes

Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs
Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs
Comment threadsrc/EFCore.PG/Metadata/NpgsqlIndexAnnotations.cs

austindrenski approved these changes Oct 17, 2018
View reviewed changes

austindrenski added this to the 2.2.0 milestone Oct 19, 2018

austindrenski mentioned this pull request Oct 22, 2018
Error when applying migration that creates a GIST index on a CIDR type #676
Closed

khellang added 2 commits November 13, 2018 15:25

Add support for index operators
0e9f625

Make sure we support schema-qualified operations
69da52f

khellang force-pushed the index-operators branch from d9a106d to 3c1f02aCompare November 13, 2018 14:27

Make INpgsqlIndexAnnotations.Operators immutable
824c091

khellang force-pushed the index-operators branch from 3c1f02a to 824c091Compare November 13, 2018 14:33

austindrenski mentioned this pull request Nov 13, 2018
Support INCLUDE for indexes (PG11) #697
Closed

roji approved these changes Nov 14, 2018
View reviewed changes

austindrenski merged commit 10fb80e into npgsql:devNov 14, 2018

khellang deleted the index-operators branch November 14, 2018 19:11

This was referenced Nov 15, 2018
Add support for INCLUDE clause in indexes #699
Merged
Add scaffolding support for new index features #704
Closed

roji added a commit to roji/efcore.pg that referenced this pull request Nov 17, 2018
Scaffold index operators
eb5efd7
See npgsql#662, npgsql#704

roji added a commit to roji/efcore.pg that referenced this pull request Nov 17, 2018
Scaffold index operators
7e3ed52
See npgsql#662, npgsql#704

roji added a commit that referenced this pull request Nov 17, 2018
Scaffold index operators
35389ee
See #662, #704

austindrenski pushed a commit to austindrenski/npgsql-efcore.pg that referenced this pull request Nov 18, 2018
Scaffold index operators
d37c308
See npgsql#662, npgsql#704

Uh oh!

Conversation

khellang commented Oct 9, 2018

Uh oh!

austindrenski left a comment

Choose a reason for hiding this comment

Uh oh!

austindrenskiOct 9, 2018

Choose a reason for hiding this comment

Uh oh!

austindrenskiOct 9, 2018

Choose a reason for hiding this comment

Uh oh!

austindrenskiOct 9, 2018

Choose a reason for hiding this comment

Uh oh!

khellang commented Oct 9, 2018

Uh oh!

austindrenski commented Oct 9, 2018

Uh oh!

khellang commented Oct 9, 2018

Uh oh!

austindrenski left a comment

Choose a reason for hiding this comment

Uh oh!

khellang commented Oct 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

austindrenski commented Oct 11, 2018

Uh oh!

khellang commented Oct 11, 2018

Uh oh!

khellang commented Oct 11, 2018

Uh oh!

austindrenski commented Oct 11, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

austindrenski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

austindrenski commented Oct 19, 2018

Uh oh!

austindrenski commented Nov 12, 2018

Uh oh!

khellang commented Nov 13, 2018

Uh oh!

austindrenski commented Nov 13, 2018

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

khellang commented Oct 10, 2018•
edited
Loading