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

[core] [macros] Add optional support for compact_str #1922

Closed
wants to merge 2 commits into from

Conversation

mcronce
Copy link
Contributor

@mcronce mcronce commented Jun 22, 2022

This is one of several "small string optimization" crates out there; I use it in particular because it seems to seems to effectively optimize both performance and size, including Option. Being able to pull short strings from a database without an extra allocation for each is a nice little win.

I wasn't able to make a generic impl Encode work (e.g. impl<'q, DB> Encode<'q, DB> for CompactString where DB: Database) due to lifetime issues without allocating a heap String temporarily, so I have separate impls for each database. Decode was no problem, though, and that makes me think I was probably missing something with Encode...

@mcronce
Copy link
Contributor Author

mcronce commented Jun 22, 2022

I also wasn't able to make query_as! work with CompactString struct fields without doing AS "...: CompactString" in the query - that seems like it's just a limitation of the macros, but I'd love to be told I'm wrong there as well :)

@mcronce mcronce changed the title [core] Add optional support for compact_str [core] [macros] Add optional support for compact_str Jun 22, 2022
@mcronce
Copy link
Contributor Author

mcronce commented Jun 22, 2022

Never mind that bit about query_as!, I noticed something in grep output that I must have overlooked a dozen times and got it working

@abonander
Copy link
Collaborator

Instead of adding support for arbitrary string-wrapper crates, as upgrading to new versions of compact_str would be a breaking change, I'd rather look into ways to make it easier to create this wrapper type in your own code using #[derive(sqlx::Type)].

#[derive(sqlx::Type)]
#[sqlx(transparent, display_fromstr)]
pub struct MyStr(CompactString);

And then with some improvements to query_as! planned in the near-future, you wouldn't need to use a type override in the query as it would just validate that MyStr is compatible with SQL type TEXT and accept the mapping as-is.

Alternatively this might also work as a control attribute on #[derive(sqlx::FromRow)] like was being worked on in #1081, which I regret leaving open for so long.

@mcronce
Copy link
Contributor Author

mcronce commented Jun 22, 2022

@abonander that sounds substantially better than what I came up with ;)

@abonander abonander closed this Jul 7, 2022
@abonander abonander mentioned this pull request Jul 28, 2022
@lcmgh
Copy link

lcmgh commented Nov 11, 2022

#[derive(sqlx::Type)]
#[sqlx(transparent, display_fromstr)]
pub struct MyStr(CompactString);

Hi @abonander . I am wondering if the chances for ending up on the stack remain the same for MyStr(CompactString) as for CompactString?

@mcronce
Copy link
Contributor Author

mcronce commented Dec 29, 2022

@abonander so, trying to use #[sqlx(try_from = "String")] in a new project and it appears it doesn't work with query_as!. Can we revisit this?

@CosmicHorrorDev
Copy link
Contributor

CosmicHorrorDev commented Dec 29, 2022

@mcronce Snowed in so can't personally check, but did you try using the custom type override as detailed here?

https://docs.rs/sqlx/latest/sqlx/macro.query.html#force-a-differentcustom-type

@mcronce
Copy link
Contributor Author

mcronce commented Jan 17, 2023

@CosmicHorrorDev that requires Decode and Encode impls

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.

4 participants