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

feat: generic query building #5127

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Conversation

jacek-prisma
Copy link
Contributor

@jacek-prisma jacek-prisma commented Jan 15, 2025

Implements a generic SqlQueryBuilder for all quaint visitors for the QueryBuilder trait, and uses the QueryBuilder trait in the compiler

@jacek-prisma jacek-prisma requested a review from a team as a code owner January 15, 2025 12:58
@jacek-prisma jacek-prisma requested review from aqrln and removed request for a team January 15, 2025 12:58
#[error("{0}")]
QuaintError(#[from] quaint::error::Error),
#[error("query builder error: {0}")]
QueryBuildFailure(#[source] Box<dyn std::error::Error + Send + Sync>),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made the error dynamic too, it might be odd to use a dynamic query builder with non-dynamic errors, but I'm open to suggestions


pub use query_arguments_ext::QueryArgumentsExt;

pub trait QueryBuilder {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this trait might need to change, it's pretty low level and makes some assumptions but I think it's an OK start

.collect()
} else {
let partitioned_batches = partition_into_batches(args, ctx);
trace!("Total of {} batches to be executed.", partitioned_batches.len());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this trace, not sure if we'll need it since this can be introspected pretty easy now that query building is separated, but I can add it back, it'll require depending on tracing in the builder crate though

Copy link

codspeed-hq bot commented Jan 15, 2025

CodSpeed Performance Report

Merging #5127 will degrade performances by 5.51%

Comparing feat/dynamic-query-builder (b013cb8) with compiler (6b19a39)

Summary

❌ 1 regressions
✅ 10 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark compiler feat/dynamic-query-builder Change
build (medium) 2.5 ms 2.6 ms -5.51%

Copy link
Contributor

github-actions bot commented Jan 15, 2025

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.156MiB 2.146MiB 10.146KiB
Postgres (gzip) 864.564KiB 860.921KiB 3.643KiB
Mysql 2.196MiB 2.185MiB 10.764KiB
Mysql (gzip) 874.813KiB 871.827KiB 2.987KiB
Sqlite 2.157MiB 2.147MiB 10.296KiB
Sqlite (gzip) 858.995KiB 855.655KiB 3.340KiB

@@ -191,49 +167,6 @@ pub(crate) async fn create_record(
}
}

/// Returns a set of fields that are used in the arguments for the create operation.
fn collect_affected_fields(args: &[WriteArgs], model: &Model) -> HashSet<ScalarFieldRef> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this and one other function to the sql builder crate, might wanna sync this with main

@@ -39,6 +39,7 @@ serial_test = "*"
quaint.workspace = true
indoc.workspace = true
indexmap.workspace = true
sql-query-builder = { path = "../query-builders/sql-query-builder" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dev-dependency for the compiler example

Copy link
Member

Choose a reason for hiding this comment

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

Let's move the compiler example to the top-level query-compiler crate when it's ready (not in this PR)

Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

Looks great, we'll probably iterate on the interface as we go but it's a very good start.

@jacek-prisma jacek-prisma merged commit 5f0c232 into compiler Jan 16, 2025
338 of 364 checks passed
@jacek-prisma jacek-prisma deleted the feat/dynamic-query-builder branch January 16, 2025 14:24
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.

2 participants