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.

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

Port DatasetExtensions to .NET Core #19771

Closed
hugosmarques opened this issue May 15, 2017 · 77 comments

Comments

Projects
None yet
@hugosmarques
Copy link

commented May 15, 2017

Anyones knows if .AsEnumerable() propertie will be available for datatable on .net core

@danmosemsft

This comment has been minimized.

Copy link
Member

commented May 15, 2017

This is a request for DataTableExtensions.AsEnumerable which is in System.Data.DataSetExtensions.dll.

@saurabh500 is this available for Core? I don't see it in our sources or NuGet.

@saurabh500

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@danmosemsft DataSetExtensions was not ported to Core.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented May 15, 2017

Is it something worth considering - would it be a fit for CoreFX? We have had success with dumping desktop sources and community folk driving the work.

This is the only ask I have seen for it though.

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 15, 2017

@saurabh500 did you know if it will be available in the future? I have to know because if this is not available I have to abandon the .net approach.

@saurabh500

This comment has been minimized.

Copy link
Member

commented May 15, 2017

The source for DataSetExtensions is at https://github.com/Microsoft/referencesource/tree/master/System.Data.DataSetExtensions

We haven't planned porting this particular assembly to .Net Core. However I do agree with @danmosemsft idea about having the community bring the code to .Net Core. I don't see any platform specific dependencies in the code so this should be relatively simple.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented May 15, 2017

Would we be open @saurabh500 to having it in CoreFX? If so, we can invite a PR to get things started.

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 15, 2017

Well guys I do not have words for your good vibe in helping me. I will be very thankful if this is implemented, because right now for me is impossible to Port all my code to EF, I have thousands of line code and I use datatable with Linq. If is simple as @saurabh500 says I'm praying that it's implemented.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@karelz fyi another port-to-core request.

@karelz karelz added the enhancement label May 15, 2017

@karelz karelz changed the title Datatable .AsEnumerable() DataTable .AsEnumerable() May 15, 2017

@karelz karelz added this to the Future milestone May 15, 2017

@karelz

This comment has been minimized.

Copy link
Member

commented May 15, 2017

Moving to Future.

Next step: If there are folks interested, we can dump the code and let community make the code build, add tests, then ship it. Marking as up-for-grabs.

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 16, 2017

If the integration of this is relatively simple i hope that someone do this ASAP please.

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 16, 2017

is not supose that if .net core have DataTables properties gave the AsEnumerable propertie too, otherwise how i can use LINQ with datatables

@karelz

This comment has been minimized.

Copy link
Member

commented May 16, 2017

@saurabh500 @NickCraver @FransBouma do you think this is must-have for 2.0? Or can it wait for Future?

@FransBouma

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2017

I use it with meta-data filled DataTable instances (obtained from e.g. GetSchemaTable, stored procedures or ad-hoc sql queries) so I can write Linq queries consuming the meta-data. I use this method as it was available on in .NET full (the code is in .NET full targeting drivers), however I think datatable.Rows.Cast<DataRow>() would do just as well as .AsEnumerable(). EnumerableRowCollection, the class handling the enumeration of AsEnumerable(), has more logic than just a yield return or a cast of Rows, but in AsEnumerable() none of that is used, so it basically comes down to datatable.Rows.Cast() (as far as I can see by reading decompiled code, not tested with running code so I could be mistaken!)

So TL;DR: no, not a must have, as the workaround is .Rows.Cast(). I think the group of people targeting a DataTable with Linq is pretty small (ORM devs, using metadata in datatables with linq), and therefore there's likely not a lot of people hurt by this missing method, but as it's a simple .Rows.Cast(), it might not be a burden to port the method over as that instead of the whole EnumerableRowCollection class.

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 16, 2017

@FransBouma tell me one thing. I see that i can use:

datatable.Select().Select(x => ... instead of datatable.AsEnumerable().Select(x => x.....

Exist any diference between the two cases?

@karelz

This comment has been minimized.

Copy link
Member

commented May 16, 2017

Thanks for advice @FransBouma!

@FransBouma

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2017

I see that datatable.Select() creates a new array and datatable.AsEnumerable() wraps the rows in the datatable, similar to datatable.Rows.Cast<DataRow>(), so the former copies data, the latter wraps it, which can have a huge impact.

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 16, 2017

@FransBouma so please if as you said is a very simple feature can we have this for the 2.0 version?

@FransBouma

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2017

@karelz

This comment has been minimized.

Copy link
Member

commented May 17, 2017

@hugosmarques unless it is super-critical for many customers, we will not add more APIs into .NET Core 2.0, sorry. See details in #17619 (comment)

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 17, 2017

@karelz thanks for all but i cannot understand this decision.... as we all know this is a simple integration and when .net core team decide to put the DataTable on .Net Core i did not understand why does not put the AsEnumerable propertie. People that use DataTable needs this. This is a disapoiting final for me because i cannot move forward with my migration to .Net Core without this.....

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 17, 2017

@FransBouma if i use dt.Rows.Cast().Where(a => instead of dt.AsEnumerable().Where(a =>
is the same or almost the same right?

I make a test with 1000000 rows on datatable and the time for both are the same. Did you see any incovenient of using dt.Rows.Cast() instead of dt.AsEnumerable()

@FransBouma

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2017

No, I think they're the same, functionality wise, as far as I can see.

@hugosmarques

This comment has been minimized.

Copy link
Author

commented May 17, 2017

@FransBouma this is what I need from beginning... with this I don't need to port AsEnumerable to .net core
dt.Rows.Cast() is available on .net core 2 right?

@joetherod

This comment has been minimized.

Copy link

commented Feb 27, 2018

The reference source doesnt seem to have the latest extensions code. The other source, I had trouble getting the LingDataView class to get all it needs. I think it was probably because it was 4.7 and my system,data is a different version. Are there links to the previous versions of the framework source code or do I have to download the whole thing?

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

@joetherod which code do you believe is out of date? I just diffed the latest internal .NET Framework sources with https://github.com/Microsoft/referencesource.git and the relevant sources seem to match

@joetherod

This comment has been minimized.

Copy link

commented Feb 28, 2018

This file, https://github.com/Microsoft/referencesource/tree/master/System.Data.DataSetExtensions
Where are the extension classes?
Is there a chance you port this over in the preview release of System.Data.DataSetExtensions.
And fix the nuget packages?

@savornicesei

This comment has been minimized.

Copy link

commented Feb 28, 2018

@joetherod You can find the package here along with instructions on setting up the myget feed in your environment.
I have added the System.Data.DataSetExtensions project (from this repo) into one of my projects and it complained about DataSetExtensions class so I removed it. I also replaced the whole thing with the NuGet preview package and it worked fine.

All the best,
Simo

@joetherod

This comment has been minimized.

Copy link

commented Feb 28, 2018

@savornicesei I was able to install the package, but it still doesnt have support for AsDataView for EnumerableRowCollection

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

@joetherod which specific type or file is missing for you? eg
https://github.com/Microsoft/referencesource/blob/90b323fe52bec428fe4bd5f007e9ead6b265d553/System.Data.DataSetExtensions/System/Data/LinqDataView.cs

Is there a chance you port this over in the preview release of System.Data.DataSetExtensions.

If we merge the assemblies it is possible, but priority wise you are the only person who has asked for these to date as far as I know. Is anyone else blocked on them?

@joetherod

This comment has been minimized.

Copy link

commented Feb 28, 2018

LinqDataView.cs and the extension methods for AsDataView() in DataTableExtensions.
When bringing in the LinqDataView.cs, the base class isnt in sync with my version of Dataview.cs in core.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

I linked to LinqDataView.cs above, AsDataView is here:
https://github.com/Microsoft/referencesource/blob/90b323fe52bec428fe4bd5f007e9ead6b265d553/System.Data.DataSetExtensions/System/Data/DataTableExtensions.cs#L258

I checked these sources for DataSetExtensions exactly match the .NET 4.7.1 sources. .NET Core sources are often different at least for formatting.

@joetherod

This comment has been minimized.

Copy link

commented Feb 28, 2018

The code in core doesnt exist for dataextensions, so not sure what you mean. I copied over all of the classes from 4.6 and 4.7. The LinqDataView.cs has errors with it base class ctor implementation which is DataView.cs.

internal LinqDataView(
DataTable table,
Func<DataRow, bool> predicate_func,
Predicate predicate_system,
Comparison comparison,
Func<object, DataRow, int> comparerKeyRow,
SortExpressionBuilder sortExpressionBuilder)

            //Parent constructor
        : **base(table,
            predicate_system,
            comparison,
            DataViewRowState.CurrentRows)**
    {
        this.sortExpressionBuilder = (sortExpressionBuilder == null) ? this.sortExpressionBuilder : sortExpressionBuilder;    
        this.comparerKeyRow = comparerKeyRow;
    }
@danmosemsft

This comment has been minimized.

Copy link
Member

commented Mar 1, 2018

The code for DataSetExtensions is in .NET Core -- it's at https://github.com/dotnet/corefx/blob/master/src/System.Data.DataSetExtensions/src

If you copy over the code from reference source, including LinqDataView, you will hit the issue I mentioned that it requires access to internal members of the base class, and the base class is in System.Data.Common.dll. In .NET framework this was OK, because the assembly of the base class (named System.DAta.dll there) exposes its internal members to System.Data.DAtasetExtensions.dll.

Does that help? I apologize because I feel like I'm not udnerstanding well.

@joetherod

This comment has been minimized.

Copy link

commented Mar 1, 2018

@danmosemsft I understand now. So, this cant be done by me. Can someone take care of this in the prerelease package of DataSetExtensions then?
Thanks

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Mar 1, 2018

@joetherod since it requires merging the assemblies it's not trivial work and will probably not happen for 2.1 given the low usage of this API unless it emerges that a bunch of other customers are hitting it also. Hopefully you can find a workaround.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Mar 1, 2018

Since this issue is closed I opened #27610 to track it. Lte's continue any conversation there.

@divega

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

Since this issue is closed I opened #27610 to track it. Lte's continue any conversation there.

@danmosemsft actually this issue doesn't appear to be closed. Was the intention to close it?

@danmosemsft danmosemsft closed this Mar 5, 2018

@ToddBradley

This comment has been minimized.

Copy link

commented Jul 30, 2018

@danmosemsft Low usage? Every single Stack Overflow explanation of how to create a subset DataTable from an existing one relies on AsEnumerable, which doesn't exist in .NET Core. The only workaround seems to be rolling your own subsetting code in a big for loop, which is ugly and messy.

@OHAVM

This comment has been minimized.

Copy link

commented Dec 3, 2018

Since this issue is closed I opened #27610 to track it. Lte's continue any conversation there.

With respect, this is going to be a pain point for me and it appears that the other issue was closed with a "this is too hard and we're lacking resources. Sorry". I will say though, @danmosemsft , if your plan is to not implement this causing .NET Framework developers converting to .NET Core to code around the lack of Data Table extensions, causing the need for DataTableExtensions to drop off meaning that your team won't have to actually address this issue, then I have to respect your logic even if I'm at the unfortunate end of it.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

@OHAVM the only reason this was not done is convoluted dependency problems. You can see some discussion in #27610 . I have driven many ports of NET Framework features and it is pretty rare that we encounter such problems.

We don't have plans right now to have another run at the problem but I reopen this so we can continue to get feedback. As well as leaving a comment I encourage folks interested in this to do a thumbs up of the top comment. That's a datapoint we've used in the past.

@danmosemsft danmosemsft reopened this Dec 3, 2018

@MSiccDev

This comment has been minimized.

Copy link

commented Jan 11, 2019

We are currently running into issues with the AsDataView() extension method as well. One of our private third-party NuGet packages needs this method internally and is, of course, throwing MethodNotFoundException there.

Microsoft wants us to port our code base over to newer versions of .NET, but puts not only stones, but rocks in our way. In my opinion the low number of complaints is just because others have thrown the towel on porting their code base already earlier, so they never reached here. Just because you do not see a lot of telemetry data regarding to certain code parts, it does not mean they are not essential. There are effective methods to block telemetry data submission, which are often used in enterprise applications.

@bmperdue

This comment has been minimized.

Copy link

commented Mar 19, 2019

Really need the AsDataView() Added this is causing a lot of issues getting our code to work.

@don4of4

This comment has been minimized.

Copy link

commented Mar 28, 2019

@danmosemsft Based on a cursory review, it looks like this relatively simple feature is causing a lot of pain.

Could you update us on the status of this? The ticket you referenced is a dead link.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Fixed link - it was #27610. There are more details in the older PR #31764.

@divega divega removed this from the 2.1.0 milestone Mar 29, 2019

@ericstj

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Fixed in #36528

@ericstj ericstj closed this Apr 3, 2019

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

@don4of4 @hugosmarques @OHAVM @bmperdue and all other folks above looking for this package, I'd be grateful if you'd try it out in our Preview 4 release (not too far away) so that we can be sure it solves your problems (and if not , we can fix it)

Thanks @ericstj for porting.

@karelz karelz added this to the 3.0 milestone May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.