Change _SqlMetaData to lazy initialize hidden column map

[Wraith2]

This PR started as a small perf optimization to eliminate the visible column map in the _SqlMetaDataSet type but in doing so I found that it wasn't going to work as expected anyway so I've fixed it.

Some background. SQL Server can return additional hidden columns that the user didn't request and if it does this it will mark the metadata entry as hidden. I've never seen one of these and can't find any way to cause them to happen (so no tests) but the behaviour is in the TDS spec. There are only a small number of places that need to care if a column is visible and those places are all acting on entire rows rather than getting a column requested by the user, GetValues and GetSqlValues are the primary consumers with one additional use in Smi (SQL Metadata Interface?) extended metadata generation.

Currently every time the metadata is read from the TDS stream an int array is allocated and populated with a map of actual metadata to visible metadata indices. This is almost never used and is thrown away. It can't be cached usefully because it is query dependent. This PR changes the generation of that map to be lazy initialized inside the _SqlMetaDataSet type and adds some abstraction so that callers only need to know about whether they want to know about real columns or visible columns.

The way that the visible column map in the current implementation is used causes incorrect output. Consider this gist: https://gist.github.com/Wraith2/06c9391808a43f9bae729a5b52a6bd10
If I have 4 columns (0,1,2,3) and set 1 to be hidden when I call GetSqlValues I would expect to get (0,2,3), in the current implementation I do not, I get (0,2,default) and other hidden column locations give similar wrong results.

[Wraith2]

This code was creating an array of size visibleColumns and then iterating through total column count and setting at index of total column count. If a hidden column was ever present it would fall over with an out of range exception.

[johnnypham]

According to the CommandBehavior docs, hidden columns are appended to the result set so it seems like the exception would never have occurred in the original code.

[Wraith2]

Lucky. The code looked wrong and even if it was intentional that it worked it wasn't clear what it was doing was right. I can revert if you like but I think this version is better.

[johnnypham]

Agreed, your changes look good to me.

[ErikEJ]

Does hidden columns refer to temporal data? If so, it should be possible to test?

CREATE SCHEMA History;
GO

ALTER TABLE InsurancePolicy
    ADD
        SysStartTime DATETIME2 GENERATED ALWAYS AS ROW START HIDDEN
            CONSTRAINT DF_SysStart DEFAULT SYSUTCDATETIME()
      , SysEndTime DATETIME2 GENERATED ALWAYS AS ROW END HIDDEN
            CONSTRAINT DF_SysEnd DEFAULT CONVERT(DATETIME2, '9999-12-31 23:59:59'),
        PERIOD FOR SYSTEM_TIME (SysStartTime, SysEndTime);
GO

ALTER TABLE InsurancePolicy
    SET (SYSTEM_VERSIONING = ON (HISTORY_TABLE = History.InsurancePolicy));

[Wraith2]

I did find mention of them and history tables. I'm pretty sure they've existed for a while. Given that the code is pretty clearly wrong everywhere that deals with them I doubt anyone has ever really used this library around them.

[Wraith2]

I tried a temporal table and the metadata for the hidden columns isn't passed back to the client at all, so it isn't that.

[David-Engel]

@Wraith2 I figured out how to produce hidden columns. According to the TDS spec, it's for the FOR BROWSE option:

create table users (
	user_id int not null identity(1,1),
	first_name varchar(100) null,
	last_name varchar(100) null);
go

alter table users add constraint pk_users_user_id primary key (user_id);
go

insert into users (first_name,last_name) values ('Joe','Smith')
go

And then use the query:

select first_name, last_name from users for browse

[David-Engel]

SqlCommand.ExecuteReader(CommandBehavior.KeyInfo) is supposed to be like appending "FOR BROWSE" to the statement, Can you point me to a test/code which hits this path (regardless of FOR BROWSE/KeyInfo)? I think you are working with TVPs or XML somehow, but I can't figure out how these intersect with FOR BROWSE. If you can develop a test to exercise these paths, that would make me feel a lot better about these changes.

[Wraith2]

I think you are working with TVPs or XML somehow

I found this while using TVPs. they're one of the few things that escapes the standard metadata and end up in the extended metadata types. Unfortunately everyone is paying the price for this capability when they don't need to.

I'll see if I can work up a test which exercises the hidden column feature in some way using FOR BROWSE or KeyInfo now I know that they're the way to see it in action. Well done on finding that.

[cheenamalhotra]

@Wraith2
Please add test case here when you find time so we can pick up this PR in next releases :)

[Wraith2]

Test added and a minor bug fixed.

[Wraith2]

anything else needed?