Skip to content

Proof of concept for query_tuple!, similar to query! but returns a tuple instead of an anonymous record #3780

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Proof of concept for query_tuple!, similar to query! but returns a tuple instead of an anonymous record #3780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7560858
query_tuple proof of concept
Victor-N-Suadicani Mar 12, 2025
14dfae2
Add simple test
Victor-N-Suadicani Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions sqlx-macros-core/src/query/input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ enum QuerySrc {

pub enum RecordType {
Given(Type),
Tuple,
Scalar,
Generated,
}
Expand Down Expand Up @@ -65,20 +66,28 @@ impl Parse for QueryMacroInput {
args = Some(exprs.elems.into_iter().collect())
} else if key == "record" {
if !matches!(record_type, RecordType::Generated) {
return Err(input.error("colliding `scalar` or `record` key"));
return Err(input.error("colliding `scalar`, `tuple` or `record` key"));
}

record_type = RecordType::Given(input.parse()?);
} else if key == "scalar" {
if !matches!(record_type, RecordType::Generated) {
return Err(input.error("colliding `scalar` or `record` key"));
return Err(input.error("colliding `scalar`, `tuple` or `record` key"));
}

// we currently expect only `scalar = _`
// a `query_as_scalar!()` variant seems less useful than just overriding the type
// of the column in SQL
input.parse::<syn::Token![_]>()?;
record_type = RecordType::Scalar;
} else if key == "tuple" {
if !matches!(record_type, RecordType::Generated) {
return Err(input.error("colliding `scalar`, `tuple` or `record` key"));
}

// we currently expect only `tuple = _`
input.parse::<syn::Token![_]>()?;
record_type = RecordType::Tuple;
} else if key == "checked" {
let lit_bool = input.parse::<LitBool>()?;
checked = lit_bool.value;
Expand Down
7 changes: 6 additions & 1 deletion sqlx-macros-core/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ pub fn expand_input<'a>(
database_url_parsed,
..
} => Err(format!(
"no database driver found matching URL scheme {:?}; the corresponding Cargo feature may need to be enabled",
"no database driver found matching URL scheme {:?}; the corresponding Cargo feature may need to be enabled",
database_url_parsed.scheme()
).into()),
QueryDataSource::Cached(data) => {
Expand Down Expand Up @@ -316,6 +316,11 @@ where

record_tokens
}
RecordType::Tuple => {
let columns = output::columns_to_rust::<DB>(&data.describe)?;

output::quote_query_tuple::<DB>(&input, &query_args, &columns)
}
RecordType::Given(ref out_ty) => {
let columns = output::columns_to_rust::<DB>(&data.describe)?;

Expand Down
65 changes: 65 additions & 0 deletions sqlx-macros-core/src/query/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,71 @@ pub fn quote_query_as<DB: DatabaseExt>(
}
}

pub fn quote_query_tuple<DB: DatabaseExt>(
input: &QueryMacroInput,
bind_args: &Ident,
columns: &[RustColumn],
) -> TokenStream {
let instantiations = columns.iter().enumerate().map(
|(
i,
RustColumn {
var_name, type_, ..
},
)| {
match (input.checked, type_) {
// we guarantee the type is valid so we can skip the runtime check
(true, ColumnType::Exact(type_)) => quote! {
// binding to a `let` avoids confusing errors about
// "try expression alternatives have incompatible types"
// it doesn't seem to hurt inference in the other branches
#[allow(non_snake_case)]
let #var_name: #type_ = row.try_get_unchecked::<#type_, _>(#i)?.into();
},
// type was overridden to be a wildcard so we fallback to the runtime check
(true, ColumnType::Wildcard) => quote! (
#[allow(non_snake_case)]
let #var_name = row.try_get(#i)?;
),
(true, ColumnType::OptWildcard) => {
quote! (
#[allow(non_snake_case)]
let #var_name = row.try_get::<::std::option::Option<_>, _>(#i)?;
)
}
// macro is the `_unchecked!()` variant so this will die in decoding if it's wrong
(false, _) => quote!(
#[allow(non_snake_case)]
let #var_name = row.try_get_unchecked(#i)?;
),
}
},
);

let var_name = columns.iter().map(|col| &col.var_name);

let db_path = DB::db_path();
let row_path = DB::row_path();

// if this query came from a file, use `include_str!()` to tell the compiler where it came from
let sql = if let Some(ref path) = &input.file_path {
quote::quote_spanned! { input.src_span => include_str!(#path) }
} else {
let sql = &input.sql;
quote! { #sql }
};

quote! {
::sqlx::__query_with_result::<#db_path, _>(#sql, #bind_args).try_map(|row: #row_path| {
use ::sqlx::Row as _;

#(#instantiations)*

::std::result::Result::Ok((#(#var_name),*))
})
}
}

pub fn quote_query_scalar<DB: DatabaseExt>(
input: &QueryMacroInput,
bind_args: &Ident,
Expand Down
16 changes: 15 additions & 1 deletion src/macros/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
/// | At Least One | `.fetch(...)` | `impl Stream<Item = sqlx::Result<{adhoc struct}>>` | Call `.try_next().await` to get each row result. |
/// | Multiple | `.fetch_all(...)` | `sqlx::Result<Vec<{adhoc struct}>>` | |
///
/// \* All methods accept one of `&mut {connection type}`, `&mut Transaction` or `&Pool`.
/// \* All methods accept one of `&mut {connection type}`, `&mut Transaction` or `&Pool`.
/// † Only callable if the query returns no columns; otherwise it's assumed the query *may* return at least one row.
/// ## Requirements
/// * The `DATABASE_URL` environment variable must be set at build-time to point to a database
Expand Down Expand Up @@ -675,6 +675,20 @@ macro_rules! query_scalar (
)
);

/// A variant of [`query!`][`crate::query!`] which returns a tuple instead of an anonymous row type.
///
/// See [`query!`][`crate::query!`] for more information.
#[macro_export]
#[cfg_attr(docsrs, doc(cfg(feature = "macros")))]
macro_rules! query_tuple (
($query:expr) => (
$crate::sqlx_macros::expand_query!(tuple = _, source = $query)
);
($query:expr, $($args:tt)*) => (
$crate::sqlx_macros::expand_query!(tuple = _, source = $query, args = [$($args)*])
)
);

/// A variant of [`query_scalar!`][`crate::query_scalar!`] which takes a file path like
/// [`query_file!`][`crate::query_file!`].
#[macro_export]
Expand Down
17 changes: 17 additions & 0 deletions tests/postgres/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,23 @@ async fn test_query() -> anyhow::Result<()> {
Ok(())
}

#[sqlx_macros::test]
async fn test_query_tuple() -> anyhow::Result<()> {
let mut conn = new::<Postgres>().await?;

let account = sqlx::query_tuple!(
"SELECT * from (VALUES (1, 'Herp Derpinson')) accounts(id, name) where id = $1",
1i32
)
.fetch_one(&mut conn)
.await?;

assert_eq!(account.0, Some(1));
assert_eq!(account.1.as_deref(), Some("Herp Derpinson"));
Comment on lines +35 to +36
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly why I don't think we should support this. The context is immediately and entirely lost. What if there's 50+ lines of code between the query and where a value is used? It won't be obvious at all what account.0 or account.1 is supposed to be.

If the idea was to immediately destructure the tuple into separate bindings, like an augmented query_scalar!(), I could maybe understand that:

let (account_id, account_name) = sqlx::query_tuple!(...).fetch_one(&mut conn).await?

However, even that loses the context that they're all connected to a single (logical if not physical) database record--unless you prefix the names like I did here, but even then, account_id is the same number of characters as account.id.

I don't see the value here. Honestly, I think this would make for less maintainable code than the status quo.

Copy link
Author

@Victor-N-Suadicani Victor-N-Suadicani Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand where you're coming from, but I do think this could have its place. It sounds like a general argument against tuples, not so much an argument about tuples specifically in sqlx.

Don't get me wrong, I agree that tuples are not as precise as structs with named fields. That's why we don't just use tuples everywhere. But tuples do have their place; there is a reason that tuples are in the language, and I think it could be reasonably used within sqlx as well. Certainly not always, but sometimes, much the same way as tuples are used generally in Rust 🙂.

I think the example you give with destructuring is a very nice usage for instance. I mean, I could definitely imagine situations where this...

let (account_id, account_name) = sqlx::query_tuple!(...).fetch_one(&mut conn).await?;
println!("Account ID: {}", account_id);
println!("Account name: {}", account_name);

... would be preferable in comparison to this:

let row = sqlx::query!(...).fetch_one(&mut conn).await?;
println!("Account ID: {}", row.account_id);
println!("Account name: {}", row.account_name);

I mean, when you look at it in that light, there really is not much of a difference, but the tuple version is sometimes preferable I would say, just the same way as tuples are sometimes preferable in other situations with Rust types.

Another way to think of it: Think of query_tuple! as a kind of compromise between query_scalar! and query!. Tbh, if we had query_tuple!, maybe query_scalar! wouldn't even be necessary as you could just fetch a tuple of a single element and it would be effectively equivalent.

I'd also like to point out that with your line of reasoning, you could argue that FromRow should not be implemented for tuples. But it is, and it leads to a kind of weird asymmetry in the API, where I can use query_as (the function) to get a tuple, but there is no way to use the checked macros to get a tuple.

// If this is okay...
let (account_id, account_name): (i32, String) = sqlx::query_as(...).fetch_one(&mut conn).await?;
// ... then why is this bad?
let (account_id, account_name) = sqlx::query_tuple!(...).fetch_one(&mut conn).await?;

// Or in other words: If query_as for tuples is provided,
// why isn't there an equivalent way to get the same thing with macros?

// Also note that query_tuple! is much better than query_as in this situation
// as it checks the query at compile-time and you don't even need to specify
// the type of the tuple, as you need to do in the query_as case.

With your reasoning, it would at least be consistent if FromRow was not implemented for tuples and you had to use a named type. But that is not the case and I do feel that is the right choice, because sometimes tuples is a more convenient option. In that vein, I think the right choice would be to allow the same usage for the macro case, hence providing query_tuple :)

I hope that makes sense and I hope you can see where I'm coming from :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonander it's been a week so hope you don't mind the ping 🙂. Did you have a chance to read my comment above? Do you have any thoughts on the asymmetry in the API between query_as (the function) and query! (the macro) with regards to tuples?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinging me doesn't get me to respond faster. I already get notifications for discussions across the whole repository. I have a full-time job that isn't SQLx and it's kept me very busy lately.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally fair, take your time 🙏


Ok(())
}

#[sqlx_macros::test]
async fn test_non_null() -> anyhow::Result<()> {
let mut conn = new::<Postgres>().await?;
Expand Down
Loading