Exposing internal virtuals involving searching a key in DataView

[Anipik]

Rationale

Winforms require DataTable.AsDataView extension methods. They uses LinqViewData datatype which override some internal virtuals like

internal virtual int FindByKey(object key);
internal virtual int FindByKey(object[] key);
internal virtual DataRowView[] FindRowsByKey(object[] key);
internal virtual void SetIndex(string newSort, DataViewRowState newRowStates, IFilter newRowFilter);

So we need to expose these apis and constructor.

Api Proposal

ctor

protected DataView(DataTable table, System.Predicate<DataRow> predicate, System.Comparison<DataRow> comparison, DataViewRowState RowState);

Methods

protected virtual int FindByKey(object key);
protected virtual int FindByKey(object[] key);
public virtual DataRowView[] FindRowsByKey(object[] key);
public virtual void SetIndex(string newSort, DataViewRowState newRowStates, IFilter newRowFilter);
public Range FindRecords<TKey, TRow>(Index.ComparisonBySelector<TKey, TRow> comparison, TKey key) where TRow : DataRow { throw null; }
public Predicate<DataRow> RowPredicate { get { throw null; } set { } }
public DataRowView[] GetDataRowViewFromRange(System.Data.Range range) { throw null; }
public ComponentModel.PropertyDescriptor GetSortProperty() { throw null; }
public ComponentModel.ListSortDescriptionCollection GetSortDescriptions() { throw null; }
public System.Comparison<DataRow> SortComparison { get { throw null; } set { } }

public interface IFilter {}

public sealed class Index
{
    public delegate int ComparisonBySelector<TKey, TRow>(TKey key, TRow row) where TRow : DataRow;
}
public struct Range
{
    public Range(int min, int max) { }
    public int Count { get { throw null; } }
    public bool IsNull { get { throw null; } }
    public int Max { get { throw null; } }
    public int Min { get { throw null; } }
}

Behavior

https://referencesource.microsoft.com/#System.Data.DataSetExtensions/System/Data/LinqDataView.cs,18

implementation

https://github.com/Anipik/corefx/tree/debugAssert

Related Issue:- https://github.com/dotnet/corefx/issues/27610

cc @danmosemsft @ericstj @JeremyKuhne @divega @keeratsingh

Note Explaining Why

https://github.com/dotnet/corefx/issues/27610#issuecomment-410090277

[danmoseley]

@Anipik can you briefly add a note explaining why you have concluded we have to make these public? The thread with @ericstj

[Anipik]

I added the eric comment in the description

[JeremyKuhne]

I added the eric comment in the description

Please link to the comment directly.

[ericstj]

I take it that this is just a precursor to exposing these in netstandard? Technically you could do it without new public api if you push the types down on netcoreapp.

[Anipik]

Technically you could do it without new public api if you push the types down on netcoreapp.

i didnt understand that. can you please explain a little bit more ?

I take it that this is just a precursor to exposing these in netstandard?

yep

[ericstj]

Imagine the implementation of DataTableExtensions (and its closure) was typeforwarded to the same assembly as DataView. It wouldn’t need those virtuals to be public since it could override them from the same assembly. Granted you still wouldn’t be able to do this in a netstandard implementation, only for netcoreapp. That’s the same as this in a way, since you can only add this API to netcoreapp. I bet pushing the types down is a much larger change than this but it’s possible if you really don’t want to make that API public.

[Anipik]

Thanks for the info.
So whts the final decision here ?
should we made them public or add typeForwards to DataTableExtensions ?

[ericstj]

I think making public (actually protected unless there's a good reason for public) is better since it preserves the type factoring/architecture. I think you'll need to examine if making that change is the correct thing for the API and doesn't cause any harm or confusion for its existing use cases. Typically we try to go over that sort of thing in the API review process. If it passes that test then we go with it. If not then you fall-back to the moving-types approach.

[danmoseley]

@Anipik can these be protected rather than public?

@keeratsingh do you have any concerns about exposing these? It does seem less ugly and more correct than internalsvisibleto, but it's not our type.

[Anipik]

we can make both the properties and ctor protected. There shouldnt be any issue with that. i will test the changes in the local branch and let you know, if i face any other problems.

[keeratsingh]

@danmosemsft I believe @divega would be the better person to answer this question since he has more API related insight than I do.

[Anipik]

@danmosemsft we can make everything protected except setIndex. It is being used in other classes which doesnot inherit the dataview.
https://github.com/Anipik/corefx/commit/1a1b4ef3eb050a695a8f95c6eff68bf8985a18dc
and we have to make Ifilter interface public

I updated the proposal. I will start adding the LinqDataView to check if we require anything else

[danmoseley]

I suggest renaming IFilter to IDataRowFilter in the proposal. IFilter is much to generic a name to make public. We try to make type names unique in System.* (at least we try)

[Anipik]

@danmosemsft I have updated the issue with all the classes and methods. I also linked to the implementation branch.

[danmoseley]

I think this is going to be too much new API to expose just for this purpose.

What I suggest is

1. we just implement AsDataView directly onto DataTable. That helps new compilations. Even if the extension method is visible, the compiler will prefer it on the type. We don't need a new configuration -- it builds for netcoreapp already.
2. we add a netcoreapp configuration of System.Data.DatasetExtensions.dll, and on that, we add AsDataView implemented as a call to the method we added in (1). That helps existing built binaries that expect to find it on DataSetExtensions. We need a new configuration because when building a net standard library, the library will be consuming DataTable in its net standard state, and the implementation won't be there.

This implies that it will probably become part of .NET Standard in due course, which seems fine.

@weshaggard does this seem right.

[weshaggard]

@danmosemsft that sounds reasonable and should work if we feel this one method is worth the hassle.

[Anipik]

@danmosemsft did you mean that we expose
AsDataView method in System.Data.Common as well as in system.Data.DatasetExtensions ?

[danmoseley]

@Anipik yes. Note that if the compiler is presented with two methods with the same name and parameters, one of them an extension method to type X and the other one an regular method on type X, it will pick the regular method without complaining. So it is OK to have it in both places. The one on S.D.DSE should call directly to the one in S.D.C. Also as mentioned, you will need to add a netcoreapp configuration to the src of S.D.DSE and only expose the method there.

[danmoseley]

I would put tests in the test library for S.D.C plus just have a single test on S.D.DSE to check that it calls through to the implementation in S.D.C.

[Anipik]

@weshaggard @danmosemsft @ericstj
there is a AsDataview extension function on EnumerableRowCollection. This class is present in System.Data.DatasetExtensions. what should we do about that ?

[Anipik]

There are 2 options left here

• We can proceed with @danmosemsft suggestion plus moving EnumerableRowCollection to S.D.C and adding typeforwards for this class in S.D.DSE.
• Moving both the EnumerableRowCollection and DataTableExtension class to S.D.C and adding typeforwards for both the classes in S.D.DSE. In this case we will only need to expose the AsDataView as an extension method.

LMK in which direction should i proceed

cc @weshaggard @ericstj

[weshaggard]

@Anipik do you have any any sense of what all would need to be pulled into S.D.C in both of those options? Would it be all of S.D.DSE? If so do we know how much of a binary size increase that causes?

If both options end up with close to the same moved into S.D.C then I would go with option 2 so we remain essentially the same from the public surface area standpoint and we don't need to add any new instance members on DataTable.

[Anipik]

Would it be all of S.D.DSE? If so do we know how much of a binary size increase that causes?

I will try it and let you know how much stuff we need to pull into S.D.C

[Anipik]

@weshaggard we need to move
System\Data\DataTableExtensions.cs
System\Data\EnumerableRowCollection.cs
System\Data\SortExpressionBuilder.cs
System\Data\DataSetUtil.cs

for exposing both the overloads of AsDataView function. is that fine to proceed with ?

[weshaggard]

Does it add any more dependencies? If not I suspect it is probably fine to proceed with moving them.

[Anipik]

No It didn't require to add any more dependencies

[weshaggard]

To add a little more detail the approach of moving those types into System.Data.Common would only work for .NET Core and would still block us from supporting them in .NET Standard. We would end up having to duplicate those types in both libraries and condition them appropriately for the different configurations. So the value add verse the complexity doesn't seem to be worth the cost of adding these at this point. We can consider adding them in some way to .NET Core only in the future if there is enough need.