Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change _SqlMetaData to lazy initialize hidden column map #521

Merged
merged 4 commits into from
Aug 7, 2020
Merged

Change _SqlMetaData to lazy initialize hidden column map #521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8947cdf
change _SqlMetaData to lazy initialize hidden column map
Wraith2 Apr 12, 2020
d373bd1
address feedback
Wraith2 May 19, 2020
ed24c4e
add hidden column test
Wraith2 Jul 11, 2020
5fdc86f
address feedback
Wraith2 Jul 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions ...osoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/AlwaysEncryptedHelperClasses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,13 +234,9 @@ sealed internal partial class _SqlMetaDataSet
internal readonly SqlTceCipherInfoTable? cekTable; // table of "column encryption keys" used for this metadataset

internal _SqlMetaDataSet(int count, SqlTceCipherInfoTable? cipherTable)
: this(count)
{
cekTable = cipherTable;
_metaDataArray = new _SqlMetaData[count];
for (int i = 0; i < _metaDataArray.Length; ++i)
{
_metaDataArray[i] = new _SqlMetaData(i);
}
}
}

Expand Down
52 changes: 18 additions & 34 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,8 @@ internal virtual SmiExtendedMetaData[] GetInternalSmiMetaData()

if (null != metaData && 0 < metaData.Length)
{
metaDataReturn = new SmiExtendedMetaData[metaData.visibleColumns];

metaDataReturn = new SmiExtendedMetaData[metaData.VisibleColumnCount];
int returnIndex = 0;
Comment on lines +281 to +282
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, your changes look good to me.

for (int index = 0; index < metaData.Length; index++)
{
_SqlMetaData colMetaData = metaData[index];
Expand Down Expand Up @@ -318,7 +318,7 @@ internal virtual SmiExtendedMetaData[] GetInternalSmiMetaData()
length /= ADP.CharSize;
}

metaDataReturn[index] =
metaDataReturn[returnIndex] =
new SmiQueryMetaData(
colMetaData.type,
length,
Expand Down Expand Up @@ -346,6 +346,8 @@ internal virtual SmiExtendedMetaData[] GetInternalSmiMetaData()
colMetaData.IsExpression,
colMetaData.IsDifferentName,
colMetaData.IsHidden);

returnIndex += 1;
}
}
}
Expand Down Expand Up @@ -408,7 +410,7 @@ override public int VisibleFieldCount
{
return 0;
}
return (md.visibleColumns);
return md.VisibleColumnCount;
}
}

Expand Down Expand Up @@ -1143,31 +1145,6 @@ private bool TryConsumeMetaData()
Debug.Assert(!ignored, "Parser read a row token while trying to read metadata");
}

// we hide hidden columns from the user so build an internal map
// that compacts all hidden columns from the array
if (null != _metaData)
{
if (_snapshot != null && object.ReferenceEquals(_snapshot._metadata, _metaData))
{
_metaData = (_SqlMetaDataSet)_metaData.Clone();
}

_metaData.visibleColumns = 0;

Debug.Assert(null == _metaData.indexMap, "non-null metaData indexmap");
int[] indexMap = new int[_metaData.Length];
for (int i = 0; i < indexMap.Length; ++i)
{
indexMap[i] = _metaData.visibleColumns;

if (!(_metaData[i].IsHidden))
{
_metaData.visibleColumns++;
}
}
_metaData.indexMap = indexMap;
}

return true;
}

Expand Down Expand Up @@ -2623,11 +2600,11 @@ virtual public int GetSqlValues(object[] values)

SetTimeout(_defaultTimeoutMilliseconds);

int copyLen = (values.Length < _metaData.visibleColumns) ? values.Length : _metaData.visibleColumns;
int copyLen = (values.Length < _metaData.VisibleColumnCount) ? values.Length : _metaData.VisibleColumnCount;

for (int i = 0; i < copyLen; i++)
{
values[_metaData.indexMap[i]] = GetSqlValueInternal(i);
values[i] = GetSqlValueInternal(_metaData.GetVisibleColumnIndex(i));
}
return copyLen;
}
Expand Down Expand Up @@ -2966,7 +2943,7 @@ override public int GetValues(object[] values)

CheckMetaDataIsReady();

int copyLen = (values.Length < _metaData.visibleColumns) ? values.Length : _metaData.visibleColumns;
int copyLen = (values.Length < _metaData.VisibleColumnCount) ? values.Length : _metaData.VisibleColumnCount;
int maximumColumn = copyLen - 1;

SetTimeout(_defaultTimeoutMilliseconds);
Expand All @@ -2984,12 +2961,19 @@ override public int GetValues(object[] values)
for (int i = 0; i < copyLen; i++)
{
// Get the usable, TypeSystem-compatible value from the internal buffer
values[_metaData.indexMap[i]] = GetValueFromSqlBufferInternal(_data[i], _metaData[i]);
int fieldIndex = _metaData.GetVisibleColumnIndex(i);
values[i] = GetValueFromSqlBufferInternal(_data[fieldIndex], _metaData[fieldIndex]);

// If this is sequential access, then we need to wipe the internal buffer
if ((sequentialAccess) && (i < maximumColumn))
{
_data[i].Clear();
_data[fieldIndex].Clear();
if (fieldIndex > i && fieldIndex>0)
{
// if we jumped an index forward because of a hidden column see if the buffer before the
// current one was populated by the seek forward and clear it if it was
_data[fieldIndex - 1].Clear();
}
}
}

Expand Down
6 changes: 0 additions & 6 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4435,7 +4435,6 @@ internal bool TryProcessAltMetaData(int cColumns, TdsParserStateObject stateObj,
metaData = null;

_SqlMetaDataSet altMetaDataSet = new _SqlMetaDataSet(cColumns, null);
int[] indexMap = new int[cColumns];

if (!stateObj.TryReadUInt16(out altMetaDataSet.id))
{
Expand Down Expand Up @@ -4479,13 +4478,8 @@ internal bool TryProcessAltMetaData(int cColumns, TdsParserStateObject stateObj,
{
return false;
}

indexMap[i] = i;
}

altMetaDataSet.indexMap = indexMap;
altMetaDataSet.visibleColumns = cColumns;

metaData = altMetaDataSet;
return true;
}
Expand Down
78 changes: 68 additions & 10 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserHelperClasses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,14 +542,17 @@ public object Clone()
internal sealed partial class _SqlMetaDataSet
{
internal ushort id; // for altrow-columns only
internal int[] indexMap;
internal int visibleColumns;

internal DataTable schemaTable;
private readonly _SqlMetaData[] _metaDataArray;
internal ReadOnlyCollection<DbColumn> dbColumnSchema;

private int _hiddenColumnCount;
private int[] _visibleColumnMap;

internal _SqlMetaDataSet(int count)
{
_hiddenColumnCount = -1;
_metaDataArray = new _SqlMetaData[count];
for (int i = 0; i < _metaDataArray.Length; ++i)
{
Expand All @@ -559,11 +562,10 @@ internal _SqlMetaDataSet(int count)

private _SqlMetaDataSet(_SqlMetaDataSet original)
{
this.id = original.id;
// although indexMap is not immutable, in practice it is initialized once and then passed around
this.indexMap = original.indexMap;
this.visibleColumns = original.visibleColumns;
this.dbColumnSchema = original.dbColumnSchema;
id = original.id;
_hiddenColumnCount = original._hiddenColumnCount;
_visibleColumnMap = original._visibleColumnMap;
dbColumnSchema = original.dbColumnSchema;
if (original._metaDataArray == null)
{
_metaDataArray = null;
Expand All @@ -586,6 +588,18 @@ internal int Length
}
}

internal int VisibleColumnCount
{
get
{
if (_hiddenColumnCount == -1)
{
SetupHiddenColumns();
}
return Length - _hiddenColumnCount;
}
}

internal _SqlMetaData this[int index]
{
get
Expand All @@ -599,10 +613,54 @@ internal _SqlMetaData this[int index]
}
}

public object Clone()
public int GetVisibleColumnIndex(int index)
{
if (_hiddenColumnCount == -1)
{
SetupHiddenColumns();
}
if (_visibleColumnMap is null)
{
return index;
}
else
{
return _visibleColumnMap[index];
}
}

public _SqlMetaDataSet Clone()
{
return new _SqlMetaDataSet(this);
}

private void SetupHiddenColumns()
{
int hiddenColumnCount = 0;
for (int index = 0; index < Length; index++)
{
if (_metaDataArray[index].IsHidden)
{
hiddenColumnCount += 1;
}
}

if (hiddenColumnCount > 0)
{
int[] visibleColumnMap = new int[Length - hiddenColumnCount];
int mapIndex = 0;
for (int metaDataIndex = 0; metaDataIndex < Length; metaDataIndex++)
{
if (!_metaDataArray[metaDataIndex].IsHidden)
{
visibleColumnMap[mapIndex] = metaDataIndex;
mapIndex += 1;
}
}
_visibleColumnMap = visibleColumnMap;
}
_hiddenColumnCount = hiddenColumnCount;
}
}

internal sealed class _SqlMetaDataSetCollection
Expand Down Expand Up @@ -649,10 +707,10 @@ internal _SqlMetaDataSet GetAltMetaData(int id)
public object Clone()
{
_SqlMetaDataSetCollection result = new _SqlMetaDataSetCollection();
result.metaDataSet = metaDataSet == null ? null : (_SqlMetaDataSet)metaDataSet.Clone();
result.metaDataSet = metaDataSet == null ? null : metaDataSet.Clone();
foreach (_SqlMetaDataSet set in _altMetaDataSetArray)
{
result._altMetaDataSetArray.Add((_SqlMetaDataSet)set.Clone());
result._altMetaDataSetArray.Add(set.Clone());
}
return result;
}
Expand Down
77 changes: 77 additions & 0 deletions src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Data;
using System.Text;
Expand Down Expand Up @@ -225,5 +226,81 @@ private static bool IsColumnBitSet(SqlConnection con, string selectQuery, int in
}
return columnSetPresent;
}

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static void CheckHiddenColumns()
{
// hidden columns can be found by using CommandBehavior.KeyInfo or at the sql level
// by using the FOR BROWSE option. These features return the column information requested and
// also include any key information required to be able to find the row containing the data
// requested. The additional key information is provided as hidden columns and can be seen using
// the difference between VisibleFieldCount and FieldCount on the reader

string tempTableName = DataTestUtility.GenerateObjectName();

string createQuery = $@"
create table [{tempTableName}] (
user_id int not null identity(1,1),
first_name varchar(100) null,
last_name varchar(100) null);

alter table [{tempTableName}] add constraint pk_{tempTableName}_user_id primary key (user_id);

insert into [{tempTableName}] (first_name,last_name) values ('Joe','Smith')
";

string dataQuery = $@"select first_name, last_name from [{tempTableName}]";

johnnypham marked this conversation as resolved.
Show resolved Hide resolved

int fieldCount = 0;
int visibleFieldCount = 0;
Type[] types = null;
string[] names = null;

using (SqlConnection connection = new SqlConnection(DataTestUtility.TCPConnectionString))
{
connection.Open();

try
{
using (SqlCommand createCommand = new SqlCommand(createQuery, connection))
{
createCommand.ExecuteNonQuery();
}

using (SqlCommand queryCommand = new SqlCommand(dataQuery, connection))
{
using (SqlDataReader reader = queryCommand.ExecuteReader(CommandBehavior.KeyInfo))
{
fieldCount = reader.FieldCount;
visibleFieldCount = reader.VisibleFieldCount;
types = new Type[fieldCount];
names = new string[fieldCount];
for (int index = 0; index < fieldCount; index++)
{
types[index] = reader.GetFieldType(index);
names[index] = reader.GetName(index);
}
}
}
}
finally
{
DataTestUtility.DropTable(connection, tempTableName);
}
}

Assert.Equal(3, fieldCount);
Assert.Equal(2, visibleFieldCount);
Assert.NotNull(types);
Assert.NotNull(names);

// requested fields
Assert.Contains("first_name", names, StringComparer.Ordinal);
Assert.Contains("last_name", names, StringComparer.Ordinal);

// hidden field
Assert.Contains("user_id", names, StringComparer.Ordinal);
}
}
}