feat(Migrations): SQL Server Filterable indexes. by laskoviymishka · Pull Request #6866 · dotnet/efcore

laskoviymishka · 2016-10-26T15:37:48Z

Please check if the PR fulfills these requirements

The code builds and tests pass (verified by our automated build checks)
Commit messages follow this format

    Implement ability to add filter to index
    - Extension for index builder
    - Index creation operation
    - Corresponding SQL generator for migration
    - Corresponding SQL reverse engeniering code for scaffolding
    - Corresponding C# reverse engeniering code for scaffolding

    Fixes #5817

Tests for the changes have been added (for bug fixes / features)
Code meets the expectations our engineering guidelines. https://github.com/aspnet/Home/wiki/Engineering-guidelines.

Please review the guidelines for CONTRIBUTING.md for more details.
Fixed #5817

AndriySvyryd

In addition to the below comments please rebase on feature/1.2.0 and run build.cmd

AndriySvyryd · 2016-10-26T20:11:36Z

        public virtual string Name { get; [param: NotNull] set; }
        public virtual ICollection<IndexColumnModel> IndexColumns { get; [param: NotNull] set; } = new List<IndexColumnModel>();
        public virtual bool IsUnique { get; [param: NotNull] set; }
+        public virtual string WhereClauses { get; set; }


Add [param: CanBeNull]

Any reason this is plural? Am not familiar with SQL Server here but in PostgreSQL it's just a standard single WHERE clause

This should be renamed in Filter. Just forgot this entry.

AndriySvyryd · 2016-10-26T20:13:06Z

            [CanBeNull] string schema = null,
-            bool unique = false)
+            bool unique = false,
+            string filter = null)


Add [CanBeNull]

AndriySvyryd · 2016-10-26T20:13:12Z

            [CanBeNull] string schema = null,
-            bool unique = false)
+            bool unique = false,
+            string filter = null)


Add [CanBeNull]

AndriySvyryd · 2016-10-26T20:31:07Z


+        /// <summary>
+        /// Gets the properties that this index is defined on.
+        /// </summary>


Filtered indexes are a SQL Server feature, so it should be stored as an annotation using extension methods. See how .IsClustered is implemented.

On a second though, other relational providers support it so it can go in .Relational

roji · 2016-10-27T07:27:18Z

                Sql);
        }

+        public override void CreateIndexOperation_with_whrere_clauses()


roji · 2016-10-27T07:27:27Z

                    IsUnique = false
                });

+        [Fact]


roji · 2016-10-27T07:29:55Z

+                    Name = "IX_People_Name",
+                    Table = "People",
+                    Columns = new[] { "Name" },
+                    IsUnique = false,


This is SQLServer-specific SQL in a provider-independent test. However, it's not terribly important as this is only a unit test (i.e. the SQL never gets sent/evaluated), so no real harm.

roji · 2016-10-27T07:34:12Z

                Sql);
        }

+        [Fact]


Shouldn't this test simply override CreateIndexOperation_with_where_clauses, defined in MigrationSqlGeneratorTestBase.cs?

roji · 2016-10-27T07:34:34Z

+                Sql);
+        }
+
+        [Fact]


Should probably be defined in MigrationSqlGeneratorTestBase.cs and overridden here (useful for other providers)

laskoviymishka · 2016-10-27T13:21:09Z

@AndriySvyryd please review latest check-in. I moved it into relational annotation but not completely sure that this proper way to use realational attributes (i tried to mimic HasName behaivior).

AndriySvyryd

You also need to add a method to RelationalIndexBuilderAnnotations so filters can be set by conventions, as well as in SqlServerIndexBuilderAnnotations that would allow to set a filter just for SQL Server in case multiple providers are used with the same model.

AndriySvyryd · 2016-10-27T18:58:22Z

+        /// <summary>
+        ///     Gets a value indicating whre clauses to specific index.
+        /// </summary>
+        string Filter { get; set; }


The properties on this interface should be read-only.
Also the fix typos in the comment or just remove it.

AndriySvyryd · 2016-10-27T19:03:06Z

                          && s.Properties.Select(diffContext.FindTarget).SequenceEqual(t.Properties),
                // ReSharper disable once ImplicitlyCapturedClosure
                (s, t) => s.IsUnique == t.IsUnique
+                          && s.Relational().Filter == t.Relational().Filter


You need to use Annotations.For(t).Filter in all of the migrations code, this allows it to read the provider-specific value

AndriySvyryd · 2016-10-27T19:05:21Z

                    Table = Annotations.For(index.DeclaringEntityType).TableName,
-                    Columns = index.Properties.Select(p => Annotations.For(p).ColumnName).ToArray()
+                    Columns = index.Properties.Select(p => Annotations.For(p).ColumnName).ToArray(),
+                    Filter = index.Relational().Filter


Use Annotations.For

You missed this one :)

laskoviymishka · 2016-10-27T19:20:27Z

@AndriySvyryd done. Please review

AndriySvyryd · 2016-10-27T19:28:00Z

+    <ProjectReference Include="..\..\src\Microsoft.EntityFrameworkCore.Relational\Microsoft.EntityFrameworkCore.Relational.csproj">
+      <Project>{6A25DF99-2615-46D8-9532-821764647EE1}</Project>
+      <Name>Microsoft.EntityFrameworkCore.Relational</Name>
+    </ProjectReference>


This project shouldn't reference .Relational

AndriySvyryd · 2016-10-27T19:29:35Z


+
+        [Fact]
+        public void Can_write_index_builder_extension_with_where_clauses()


Move this to RelationalBuilderExtensionsTest and add a similar test to RelationalMetadataExtensionsTest

divega · 2016-10-27T20:13:37Z

General question about this PR: currently by default we add the filter WHERE x IS NOT NULL for unique indexes in SQL Server (not sure what we do for SQLite). Can that become a convention that takes advantage of this feature?

AndriySvyryd · 2016-10-27T20:16:52Z

@divega Sure, I can do it after this is merged

laskoviymishka · 2016-10-28T13:10:13Z

                    Annotations.For(t).Name,
                    StringComparison.OrdinalIgnoreCase)
                          && s.IsUnique == t.IsUnique
+                          && Annotations.For(s).Filter == Annotations.For(t).Filter


@AndriySvyryd May be HasDifferences can handle this comparing? It looks like i can remove this lines

HasDifferences diffs migrations annotations, while they have the same API they are different from model annotations, the added conditions are still needed.

laskoviymishka · 2016-10-28T13:24:10Z

@AndriySvyryd done, please review

AndriySvyryd

Almost there.
After making the change below rebase again on the latest feature/1.2.0 and squash the commits

AndriySvyryd · 2016-10-28T18:01:32Z

                    Annotations.For(t).Name,
                    StringComparison.OrdinalIgnoreCase)
                          && s.IsUnique == t.IsUnique
+                          && Annotations.For(s).Filter == Annotations.For(t).Filter


HasDifferences diffs migrations annotations, while they have the same API they are different from model annotations, the added conditions are still needed.

AndriySvyryd · 2016-10-28T18:56:03Z

+        /// <summary>
+        ///     Gets a value indicating whre clauses to specific index.
+        /// </summary>
+        public string Filter


This needs to be virtual. Also correct spelling in the comment or just remove it.

roji · 2016-10-28T20:44:25Z


+                if (Index.Relational().Filter != null)
+                {
+                    lines.Add("." + nameof(RelationalIndexBuilderExtensions.HasFilter) + "(\"" + Index.Relational().Filter + "\")");


String interpolation?

roji · 2016-10-28T20:44:48Z

        /// </summary>
        public new virtual bool Name([CanBeNull] string value) => SetName(value);

+        /// <summary>


Should be HasFilter?

roji · 2016-10-28T20:46:00Z

        protected abstract IMigrationsSqlGenerator SqlGenerator { get; }

        protected virtual string Sql { get; set; }
+        [Fact]


Missing empty line

Fixed dotnet#5817

laskoviymishka · 2016-10-29T09:21:35Z

@AndriySvyryd @roji done, please review.

roji · 2016-10-29T09:39:53Z

LGTM

AndriySvyryd · 2016-10-29T21:25:23Z

Thank you for your contribution.

dnfclas added the cla-already-signed label Oct 26, 2016

rowanmiller assigned AndriySvyryd Oct 26, 2016

AndriySvyryd requested changes Oct 26, 2016

View reviewed changes

roji reviewed Oct 27, 2016

View reviewed changes

This was referenced Oct 27, 2016

Support for partial indexes npgsql/efcore.pg#116

Closed

Support for indices on arbitrary expressions #3986

Closed

laskoviymishka force-pushed the feat/FiltrableIndexes branch from 434cfc6 to 548c9a9 Compare October 27, 2016 12:26

laskoviymishka changed the base branch from dev to feature/1.2.0 October 27, 2016 12:26

AndriySvyryd requested changes Oct 27, 2016

View reviewed changes

AndriySvyryd reviewed Oct 27, 2016

View reviewed changes

laskoviymishka commented Oct 28, 2016

View reviewed changes

AndriySvyryd requested changes Oct 28, 2016

View reviewed changes

roji reviewed Oct 28, 2016

View reviewed changes

feat(Migrations): SQL Server Filterable indexes.

2e0498a

Fixed dotnet#5817

laskoviymishka force-pushed the feat/FiltrableIndexes branch from f5d5866 to 2e0498a Compare October 29, 2016 09:21

AndriySvyryd merged commit 2e0498a into dotnet:feature/1.2.0 Oct 29, 2016

roji mentioned this pull request Nov 21, 2016

Model configuration: Make filtered indexes play nice with other providers #7087

Closed

ajcvickers mentioned this pull request Mar 21, 2017

Reverse engineering: Unique Index interpreted incorrectly to give one-to-one instead of one-to-many relationship #7938

Closed

                               Sql);
                       }
+                      [Fact]



		[Fact]
		public void Can_write_index_builder_extension_with_where_clauses()

Conversation

laskoviymishka commented Oct 26, 2016

Uh oh!

AndriySvyryd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka commented Oct 27, 2016

Uh oh!

AndriySvyryd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divega commented Oct 27, 2016

Uh oh!

AndriySvyryd commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka commented Oct 28, 2016

Uh oh!

AndriySvyryd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka commented Oct 29, 2016

Uh oh!

roji commented Oct 29, 2016

Uh oh!

AndriySvyryd commented Oct 29, 2016

AndriySvyryd left a comment •

edited

Loading

AndriySvyryd Oct 27, 2016 •

edited

Loading