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

Expose column information on row #292

Closed
wants to merge 4 commits into from

Conversation

zaynetro
Copy link

@zaynetro zaynetro commented Apr 30, 2020

This is my attempt to fix #181 .

I am not sure if you have the same implementation in mind so let me know if I am
on the right track.

I was not sure how to verify the column type in tests as I couldn't construct
MySqlTypeInfo due to TypeId being crate-private.
Figured it out...

This is my attempt to fix launchbadge#181 .

I am not sure if you have the same implementation in mind so let me know if I am
on the right track.

I was not sure how to verify the column type in tests as I couldn't construct
`MySqlTypeInfo` due to `TypeId` being crate-private.
@zaynetro zaynetro changed the title Expose row columns Expose column information on row Apr 30, 2020
@mehcode
Copy link
Member

mehcode commented May 6, 2020

This is a good start. We may want to change internals enough that we can produce a reference to the columns list or perhaps produce an iterator over the column.

Sorry for it not being clear but as that issue was marked proposal, I'd also like to talk more on the design space there.

Currently we have a few different Column types floating around the code base. Generally to satisfy some internal details or the describe interface that the query macros use.

I'd like to settle on a unification of those and making Executor::describe stable and supported as it all sort of ties together.

It's probably a good idea to let each driver define it's column type and associate them to the Database trait with methods to get out common properties.

Thinking through this, SQLite column meta data isn't a normal ask so we probably wouldn't be able to have a generic interface return &[Column]. But an iterator could work.

I would assume someone wanting a descriptive list of columns likely isn't in a performance sensitive scenario.

@zaynetro
Copy link
Author

zaynetro commented May 6, 2020

Thanks for taking a look at this pr!

I am a building a hobby MySQL client so my use case involves getting columns names for each executed query. Can't speak about other use cases.

I understand that there might not be a well defined interface we want to implement. How about I will try to expose column information for other databases first using the proposed interface (if possible)? Then (in the same pr) we can better evaluate whether implementation and interface make sense by looking at all database at the same time.

@zaynetro
Copy link
Author

zaynetro commented May 8, 2020

Now I see what you meant. SQlite doesn't look trivial and I didn't manage to figure out how to get type info for each column. column_decltype requires &mut self but I cannot get if from withing SqliteRow.

What are the next steps? Do we want to rethink columns interface?

Let me know what you think and whether you need more time to think about it.

@mehcode
Copy link
Member

mehcode commented May 20, 2020

@zaynetro Sorry for not getting back to you. v0.4 (should be out soon) should have the internals moved around enough where this an be trivial to add.

@zaynetro
Copy link
Author

No problem. This issue is not blocking me. Plus I have also limited time to focus on this.

@mehcode
Copy link
Member

mehcode commented Jul 5, 2020

Thank you for starting this. I've got an implementation in master. This turned out to be very involved because I wanted to include a handful of refactoring while I was touching the column information.

https://github.com/launchbadge/sqlx/blob/master/sqlx-core/src/row.rs#L83-L108

https://github.com/launchbadge/sqlx/blob/master/sqlx-core/src/column.rs#L4

@zaynetro
Copy link
Author

zaynetro commented Jul 5, 2020

Great! Thanks!

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

Successfully merging this pull request may close these issues.

[Proposal] Expose column information on Row
2 participants