Ignore hidden columns (used in SQL Server system versioned tables) #8276
Conversation
| [SqlServerCondition(SqlServerCondition.SupportsSequences)] | ||
| public void Hidden_Column() | ||
| { | ||
| using (var scratch = SqlServerTestStore.Create("SqlServerE2E")) |
There was a problem hiding this comment.
Nit: I would rename the DB to be something more easily tracked to this test e.g. HiddenColumnTest.
| , SysStartTime datetime2 GENERATED ALWAYS AS ROW START HIDDEN NOT NULL | ||
| , SysEndTime datetime2 GENERATED ALWAYS AS ROW END HIDDEN NOT NULL | ||
| , PERIOD FOR SYSTEM_TIME(SysStartTime, SysEndTime) | ||
| ) |
There was a problem hiding this comment.
Nit: Personally I like the "comma at the beginning of the line" convention, but as a team we decided to put them at the end of the previous line instead.
There was a problem hiding this comment.
I will move the commas 😄
| @@ -351,6 +351,60 @@ PRIMARY KEY (Id) | |||
| } | |||
| } | |||
|
|
|||
| [ConditionalFact] | |||
| [SqlServerCondition(SqlServerCondition.SupportsSequences)] | |||
There was a problem hiding this comment.
Does this condition just happen to coincide? Or are supporting sequences and supporting hidden columns different things? I'm wondering whether we should create a new condition SqlServerCondition.SupportsHiddenColumns?
There was a problem hiding this comment.
We should create condition for SqlServer 2016
There was a problem hiding this comment.
@smitpatel I don't think this is just a SQL Server 2016 issue. See bug #5553 about supporting sequences in SQL Azure as well. This could be similar, so I would prefer to keep the flag named for the function, not the version of SQL Server.
There was a problem hiding this comment.
I will add a new condition - #5553 is getting moot, as Azure DB v11 is being deprecated
There was a problem hiding this comment.
Preferable thing is to have flag based on the function. I mistakenly thought that we may end up with multiple conditions. Instead of having a large set of conditions to specify its good to have only one. But we won't hit such issue because this conditions are +ve. Means specify the one which needs to be present.
|
@lajones I noticed the build failed, as the NuGet v2 api had some issues earlier - maybe you can remove the lookup for v2 feed during build, and only use v3 feed? |
|
Sorry, @ErikEJ, because we have a new flag we also need to update the config.json file to have |
|
@lajones Done |
| @@ -5,7 +5,8 @@ | |||
| "ElasticPoolName": "", | |||
| "SupportsSequences": true, | |||
| "SupportsOffset": true, | |||
| "SupportsMemoryOptimized": false | |||
| "SupportsMemoryOptimized": false, | |||
| "SupportsHiddenColumns": true | |||
There was a problem hiding this comment.
Shouldn't this be false as MemoryOptimized? This is our default connection string for testing and we still use SqlServer2014 which does not support hidden columns.
There was a problem hiding this comment.
I am confused now - so you use 2016 on the CI (appveyor) and 2014 on what ?? (your dev pcs) - in that case it should be false, yes (it has no impact on the functional tests, though)
There was a problem hiding this comment.
@ErikEJ - Did not realize this confusion 😄 Minimum requirement to build EF repo is SqlServer 2014 (or it could run with older version too may be).
We use SqlServer 2016 in appveyor because appveyor was failing to start sqlserver 2014 service and build failed always providing no feedback.
The normal dev work is on sql server 2014 as of now.
There was a problem hiding this comment.
@ErikEJ @smitpatel Smit is right. I was thinking this was like SupportsSequences, but it's more like SupportsMemoryOptimized and so should be false by default.
There was a problem hiding this comment.
In the meantime - I have run this test on my dev machine (which has SQL Server 2016) and it works fine.
|
@lajones Thanks! I have updated the config.json file to match your test and dev environment |
|
@ErikEJ Looks good. Could you squash all the commits together please and I can check it in for you? Thx. |
|
@lajones Done |
lajones
added
the
closed-fixed
fixes #8192