Add DataTableExtensions.AsDataView

[ericstj]

This pushes down the minimal number of types required to implement
DataTableExtensions.AsDataView.

LinqDataView is added in order to implement AsDataView, the following
types need to be pushed down in order to implement this:

• DataSetUtil
• DataTableExtensions
• EnumerableRowCollection
• SortExpressionBuilder

Moving these down requires that the following also be moved due to needing
internals access to moved types.

• EnumerableRowCollectionExtensions
• OrderedEnumerableRowCollection
• TypedTableBase
• TypedTableBaseExtensions

This leaves behind only the following types in DataSetExtensions:

• DataRowComparer
• DataRowExtensions

I could have moved those as well, but wanted to start from the minimal change and go from there.

[ericstj]

I'll add a couple tests to this, just wanted to share the factoring to get feedback in case folks suggest we do more.

[stephentoub]

I realize all of this is copy/paste so I just skimmed it and left a few random comments.

[ericstj]

@danmosemsft @divega what do you think about most of the DataSetExtensions types being available by default but having to add a package reference for DataRowComparer and DataRowExtensions? Does that seem unusual?

[ericstj]

I realize all of this is copy/paste so I just skimmed it and left a few random comments.

Thank you @stephentoub. I'll try to apply some generalized cleanup.

[divega]

@danmosemsft @divega what do you think about most of the DataSetExtensions types being available by default but having to add a package reference for DataRowComparer and DataRowExtensions? Does that seem unusual?

I would say push those two also down to System.Data.Common. System.Data.DataSetExtensions can be just about compatibility with desktop.

[divega]

cc @ajcvickers

[danmoseley]

@danmosemsft @divega what do you think about most of the DataSetExtensions types being available by default but having to add a package reference for DataRowComparer and DataRowExtensions? Does that seem unusual?

I defer to @divega on this one, we also have usage data of course if that's helpful

[divega]

I assume if we push all the implementation to System.Data.Common, we will move these files there and not src/Common/src/System/Data/. I see that as an added benefit. Putting stuff in here has led to code duplication. See #32857.

[ericstj]

We still build a netstandard implementation here https://github.com/dotnet/corefx/blob/master/src/System.Data.DataSetExtensions/src/System.Data.DataSetExtensions.csproj.

This implementation is used when the package is installed on netstandard2.0 frameworks that aren't netcoreapp3.0 or higher (eg: netcoreapp2.x, uap10.0.16299, xamarin frameworks). We could decide to stop building that in master and just copy it from servicing but it'd mean that you need to make changes in servicing branches rather than master if you wanted to change that implementation.

Note that either way I will ensure that the change does not leave behind duplicate files.

[divega]

Ok, I didn't know that. But I believe we don't build System.Data.Common as a package from master anymore. Does that make stopping building System.Data.DataSetExtensions from master make more sense?

[ericstj]

The System.Data.Common package was eliminated for an different reason, the surface area was absorbed into netstandard2.0, as well as the facade. Because of this netstandard2.0 supporting frameworks must have the dll inbox, and the netstandard targeting pack included a facade to unify the dll to types in netstandard.dll. As such, shipping a package would be pretty pointless since all recent frameworks had to have the DLL inbox.

Contrast that to DataSetExtensions: we aren't getting rid of this DLL. We still need reference assembly for netstandard, we need facade for netcoreapp3.0, and implementations for other frameworks.

If you eliminated the package you'd still need to determine how to deliver the DataSetExtensions facade: put it inbox in .NETCore 3.0? If you're willing to do this, that'd work.

If not, then wait for next release to eliminate the package in master. That way you can use the 3.0 servicing branch to service the downlevel implementations while still producing the facade needed for 3.0.

[divega]

Makes sense. Thanks for the detailed explanation.

[divega]

put it inbox in .NETCore 3.0? If you're willing to do this, that'd work.

Would there be any downside to this? It would mean you get compatibility with libraries built against desktop without any additional package, right?

[ericstj]

Initially I was not thrilled about adding another DLL inbox, but given that it doesn't add any dependencies and we could think of it like the desktop compat shims (mscorlib, system, system.core, etc) I feel less strongly. It also has the benefit of avoiding a compiler error when folks reference the old package and target netcoreapp3.0.
@danmosemsft @stephentoub any objections?

[danmoseley]

Size is not an issue here right?

[ericstj]

Signed crossgen'ed facades with a similar number of type forwards are 14KB so that's our upper bound. Lower on linux due to signing differences.

[danmoseley]

I have no objection.

[danmoseley]

Likewise, just added comments on trivia.

[ericstj]

Pushed changes that add tests and address feedback. Waiting for folks to ack #36528 (comment) before I remove the package and reorganize the sources.

[divega]

LGTM

[ericstj]

Fixes #27610 #19771