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

support F# anonymous types #1233

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

enricosada
Copy link

@enricosada enricosada commented Apr 7, 2019

Support F# anonymous types in query like

let res = db.QuerySingle<{| Tid: int; Name: string; X:string; Y: int |}>("select Tid=20, Name='fdsf', X='hfds', Y=15")  
  • The search for constructor fallback to search by names sorted, if not found by position.
  • The instantiation of the constructor must respect order or constructor argument

To initialize the constructor argument correctly, i first tried to reoder the il fields doing the read from datareader to match constructor arguments position, not datareader name position, but i got:

System.InvalidOperationException : Invalid attempt to read from column ordinal '0'.  With CommandBehavior.SequentialAccess, you may only read from column ordinal '2' or greater.

so cannot be done like this, because read need to be sequential by column index.

To maintain the read order, before initializing the constructor, if needed, it will:

  • pop from the stack who contains the constructor args to new local variables (the order is known, because is always by column names index)
  • push back in the stack the values from the local variables, in the correct order based on constructor arguments position

fix #1226

@enricosada enricosada force-pushed the issue_1226_fsharp_anon_constructors branch from 65256db to a2d611c Compare April 8, 2019 00:01
Dapper/DefaultTypeMap.cs Outdated Show resolved Hide resolved
Dapper/SqlMapper.cs Outdated Show resolved Hide resolved
@enricosada enricosada changed the title [WIP] support F# anonymous types support F# anonymous types Apr 8, 2019
@enricosada
Copy link
Author

done, @mgravell @NickCraver ready to review

@enricosada
Copy link
Author

@mgravell anything i can help with for this PR?

@enricosada
Copy link
Author

enricosada commented Sep 25, 2019

@NickCraver @mgravell any specific issue with this PR i need to address?
happy to help to have it merged

sorry but anonymous types in f# are really handy with dapper, but are really bugged without this PR

@mgravell
Copy link
Member

I totally understand. It isn't personal - I simply haven't had chance to look yet - I'm juggling time between multiple projects. The existence of tests in the PR is great. I'll try and look for the next drop. Sorry for delays.

@enricosada
Copy link
Author

enricosada commented Sep 26, 2019

Np at all @mgravell , i know it may happen, it was just to check if there was something todo to help

meanwhile i'll rebase it

thanks a lot!

@enricosada enricosada force-pushed the issue_1226_fsharp_anon_constructors branch from d6d9d4f to c81f992 Compare October 16, 2019 18:07
@NickCraver
Copy link
Member

@enricosada did you see Marc's comment above? We'd love to get this in, but it could be a fair bit more efficient if intent matches.

@enricosada
Copy link
Author

@NickCraver no, i missed that sry, let me check. thx for ping

@enricosada
Copy link
Author

@mgravell i added some comments, because i not understood your feedback
@NickCraver happy to fix, but dunno how to improve this atm, it fix the bug atm and i didnt see a big perf hit for F#

@mgravell
Copy link
Member

mgravell commented Jan 8, 2020

Hi @enricosada - I got nudged by a mutual friend to remind me to look at this again; sorry for delay. Can I check - the confusion we're talking about - is this the zip vs dual sort vs arrays thing?

My understanding - and please do correct me if I'm wrong here - is that we have two arrays; one of the names, one of the types, matching by index; so if we have 3 columns, we might have

names: "def", "ghi", "abc"
types: Int32, String, Single

as I read the code, the intent here is to sort by the names, preserving the respective positions in both arrays, so we end up with:

names: "abc", "def", "ghi"
types: Single, Int32, String

is that correct? If so, that's exactly what the dual array version of Array.Sort does:

        string[] names = { "def", "ghi", "abc" };
        Type[] types = { typeof(int), typeof(string), typeof(float) };

        Array.Sort(names, types);

        Console.WriteLine($"names: {string.Join(", ", names)}");
        Console.WriteLine($"types: {string.Join(", ", types.Select(x => x.Name))}");

        /* output:
            names: abc, def, ghi
            types: Single, Int32, String
         */

Have I misunderstood?

@enricosada
Copy link
Author

Have I misunderstood?

hi @mgravell

Maybe i didnt understood what the dual array version of Array.Sort does.
I tried locally when you mentioned it but didnt understood.

I think that's exactly what i want, sort two arrays based on values of first array and maintaining the sync.

Let me rebase and use Array.Sort

@mgravell
Copy link
Member

mgravell commented Jan 8, 2020

sort two arrays based on values of first array and maintaining the sync

to confirm, yes: that is an accurate description of what Array.Sort does with the overload that takes two arrays

@enricosada enricosada force-pushed the issue_1226_fsharp_anon_constructors branch from c81f992 to 9bf3072 Compare January 8, 2020 14:37
@enricosada
Copy link
Author

enricosada commented Jan 8, 2020

@mgravell rebased and applied your feedback, it's now using Array.Sort instead of zip+sort
I tested locally with other cases and seems fine too.
So if appveyor is green, should be ok (the PR contains a test anyway)

Thanks a lot btw @mgravell , TIL about Array.Sort overload :D

@enricosada enricosada force-pushed the issue_1226_fsharp_anon_constructors branch from 9bf3072 to c053dfc Compare January 8, 2020 15:29
@TravisLeithVeloqx
Copy link

Can I ask about the status of this? I would like to use this feature in my project and can clone this branch for now, but only if it is expected to make it into master.

@atlemann
Copy link

I just got hit by this as well. Would be awesome if it made it in a new release!

@enricosada
Copy link
Author

@mgravell i think this is ready for review and merge.

happy to do any change required

@TravisLeithVeloqx
Copy link

I keep coming up against this issue. Is there a release schedule in mind?

@mgravell
Copy link
Member

will re-review; sorry for dropping between cracks

@mgravell
Copy link
Member

Code seems good; checking the tests locally for regressions - seems fine; you're happy that it is working from F#?

@TravisLeithVeloqx
Copy link

@mgravell @enricosada
Until now I have not actually attempted to use this.
Today I pulled this branch and built it. I added the Dapper.dll to my the project I am currently working on and attempted to use an anonymous record. I get the same parameterless default constructor error.

The offending code is

let parameters = Map ["ids", box ids; "dates", box dates]
use conn = new MySqlConnection(connectionString)

conn
|> dapperMapParametrizedQuery<{|id:int; date:DateTime; variance:float|}> sql parameters
|> Seq.groupBy (fun x -> x.date)
|> Seq.map (fun (dt, sq) -> dt, sq |> Seq.map (fun x -> {id = x.id; variance = x.variance})) |> Ok

So either I have built it wrong, or am using it wrong.

@enricosada
Copy link
Author

enricosada commented Apr 16, 2020 via email

@NickCraver NickCraver changed the base branch from master to main June 20, 2020 13:25
@travis-leith
Copy link

@enricosada is this working?

@NickCraver
Copy link
Member

NickCraver commented May 9, 2021

I updated this branch to latest main - still prepared to merge but we don't use F# and would really appreciate some community feedback on whether this works as intended.

@enricosada
Copy link
Author

@NickCraver thanks a lot.

I’ll check tomorrow morning

@brase
Copy link

brase commented Jul 21, 2021

@enricosada How can I test it? Should it work with unordered ctor parameters?

@realvictorprm
Copy link

pew, has been a while. Maybe I should take a look into that.

@realvictorprm
Copy link

@enricosada How can I test it? Should it work with unordered ctor parameters?

So general suggestion from my side, I would have gone down the route and added another test folder with an F# project that depends on Dapper and uses it with anon records to make sure that the feature is functional. Anything against going that far?
Alternatively an F# library project could be added, that contains an anon record with a type alias, such that it can be consumed by C# and used for directly piping into the C# functions to test.

@realvictorprm
Copy link

@NickCraver are you still open for having this change? If yes, could we reiterate on what you want to see from your side to be changed to get it merged, considering how long it has been since work has been done on this PR & topic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

F# Anonymous records support in query results
8 participants