Support provided type reuse

[kerams]

Implements #54. Docs and more tests coming. Possibly DataTable type reuse too. Also worth noting that the one test will break with #65 because runtime column order will become important, so that needs sorting out too. What do you think?

cc @Swoorup

[Swoorup]

So columns are sorted alphabetically and I am guessing the the same type could be shared if CreateCommand is used across boundaries(different functions)?

Pretty neat.

[kerams]

Absolutely, as long as it's CreateCommand of the same NpgsqlConnection, otherwise you run into the same problem you have with NpgsqlCommand; there's no way around it I'm afraid. Yeah, columns sorted alphabetically, because I think the select order should not matter.

[Swoorup]

Cool can't wait for this change to land. I think by default it should be turned on though :P . I can't see it breaking anything.

[daz10000]

On the runtime column order we were burned a little early on with select * behaving slightly differently between two instances of same schema. It’s convenient doing that when you’re prototyping something but we concluded in the end that we should always explicitly list column names. I think TP has code to validate column order hasn’t changed between design and runtime

Darren

Von meinem iPhone gesendet

Am 28.02.2020 um 7:10 PM schrieb Swoorup Joshi notifications@github.com:

 Cool can't wait for this change to land. I think by default it should be turned on though :P . I can't see it breaking anything.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[Swoorup]

Maybe Select * should internally always transform to expanded columns and use named bindings if that is possible?

[kerams]

@Swoorup

Maybe Select * should internally always transform to expanded columns

Internally how? Only Postgres is able to do the expansion.

I can't see it breaking anything.

It would be a breaking change for anyone referring to the old types in function signatures for example. On the other hand, there's been no V1 yet :). I'll leave it to the maintainers to decide.

@daz10000

use cmd1 = DvdRentalWithTypeReuse.CreateCommand<"select film_id, rating from film", SingleRow = true>(dvdRental)
use cmd2 = DvdRentalWithTypeReuse.CreateCommand<"select rating, film_id from film", SingleRow = true>(dvdRental)

Runtime column verification (including column order) is still performed on both of these commands. I'm merely sorting the columns in design time so that they can share the same provided type. I think it's preferable to the commands generating 2 distinct types in this case:

DvdRentalWithTypeReuse.``film_id:Int32, rating:Option<public.mpaa_rating>``
DvdRentalWithTypeReuse.``rating:Option<public.mpaa_rating>, film_id:Int32``

[kerams]

@daz10000, this is ready for review now. The behavior is turned off by default. Let me know if you want to change that.

[Swoorup]

@Swoorup

Maybe Select * should internally always transform to expanded columns

Internally how? Only Postgres is able to do the expansion.

Forgive my limited knowledge on this type providers. But if it is possible to source schema information from SQL query, wouldn't it also be possible to replace the query sent to the server? So say you have got a table like

Column Name	Data Type
Username	String
Password	String

SELECT * FROM User could always be expanded to SELECT Username, Password FROM User? But does sound like a lot of work parsing the query and replacing-in-place as select queries can be nested, used as variable and all SQL stuff. And you'd end up writing a SQL parser in this library, probably not worth it now that i think of it.

[kerams]

That could work in theory, but it would be sooooo much hassle for so little benefit. Remember that joins also affect what * expands into. In any case, users' queries should be sent to Postgres verbatim in my opinion.

[Swoorup]

Agree, other option is to produce a warning or a documentation note, elsewhere

[kerams]

@melanore, I'm assuming you'll want me to close this PR and port it once you merge the big changes?

[melanore]

If that's not too hard - it would be nice to close this PR and port it. I can also help you with porting some time tomorrow if you got your hands full. Sorry for the inconvenience.

[kerams]

I don't expect any difficulties. Will look into it tomorrow or the day after.

[Swoorup]

Did the other PR solve the select * issue ?

[kerams]

What issue do you mean? Discrepancies of column ordering for a single command?