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.

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

Jump to bottom

chore: extract query-compiler and query-compiler-wasm #5129

Merged
merged 16 commits into from
Jan 17, 2025
Merged

chore: extract query-compiler and query-compiler-wasm #5129

merged 16 commits into from
Jan 17, 2025

Conversation

jacek-prisma
Copy link
Contributor

@jacek-prisma jacek-prisma commented Jan 16, 2025 •
edited
Loading

Extracts a query compiler crate and adds a new query-compiler-wasm crate that exposes a minimal WASM API for interacting with the compiler.

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jan 16, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #5129 will not alter performance

Comparing chore/query-compiler-crate (12c4a19) with compiler (6b19a39)

Summary

✅ 11 untouched benchmarks

@jacek-prisma jacek-prisma requested a review from aqrln January 16, 2025 18:12
@jacek-prisma jacek-prisma marked this pull request as ready for review January 16, 2025 18:12
@jacek-prisma jacek-prisma requested a review from a team as a code owner January 16, 2025 18:12
aqrln
aqrln reviewed Jan 17, 2025
View reviewed changes
Makefile Show resolved Hide resolved
aqrln
aqrln reviewed Jan 17, 2025
View reviewed changes
query-compiler/query-compiler-wasm/src/lib.rs Outdated
Comment on lines 1 to 10
#[cfg(not(target_arch = "wasm32"))]
mod arch {
// This crate only works in a Wasm environment.
// This conditional compilation block is here to make commands like
// `cargo clippy --all-features` happy, as `clippy` doesn't support the
// `--exclude` option (see: https://github.com/rust-lang/rust-clippy/issues/9555).
//
// This crate can still be inspected by `clippy` via:
// `cargo clippy --all-features -p schema-engine-wasm --target wasm32-unknown-unknown`
}
Copy link
Member

Choose a reason for hiding this comment

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

I looked into this today, and I believe this shouldn't be necessary if we use a cargo feature instead of target_arch logic to conditionally apply the tsify derive macro to the structs driver-adapters crate exports.

aqrln
aqrln reviewed Jan 17, 2025
View reviewed changes
query-compiler/query-compiler-wasm/src/wasm/compiler.rs Outdated
let schema = Arc::new(psl::parse_without_validation(datamodel.into(), CONNECTOR_REGISTRY));
let schema = Arc::new(schema::build(schema, true));

tracing::info!(git_hash = env!("GIT_HASH"), "Starting query-compiler-wasm");
Copy link
Member

Choose a reason for hiding this comment

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

We might want to be able to send the tracing events to JS but let's ignore it in this PR.

aqrln
aqrln reviewed Jan 17, 2025
View reviewed changes
query-compiler/query-compiler-wasm/src/wasm/compiler.rs Outdated
pub fn compile(
&self,
request: String,
_human_readable: bool, // ignored on wasm to not compile it in
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 remove the argument completely.

aqrln
aqrln reviewed Jan 17, 2025
View reviewed changes
query-engine/driver-adapters/executor/src/demo-qc.ts
url,
schema,
}: InitQueryCompilerParams) {
const adapter = await driverAdapterManager.connect({ url });
Copy link
Member

Choose a reason for hiding this comment

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

Random thought: maybe we should put all of the client engine internals (QueryInterpreter, TransactionManager etc) into a new package, maybe even with some facade around them that ClientEngine class in @prisma/client would also use if necessary. Then we could easily import that package here to be able to run the engine tests.

@aqrln aqrln added this to the 6.3.0 milestone Jan 17, 2025
@aqrln
Copy link
Member

aqrln commented Jan 17, 2025

@jacek-prisma there are some query-engine-wasm build failures on CI:

error[E0004]: non-exhaustive patterns: `quaint::connector::SqlFamily::Postgres` not covered
   --> query-engine/connectors/sql-query-connector/src/database/js.rs:51:15
    |
51  |         match self.connection_info.sql_family() {
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pattern `quaint::connector::SqlFamily::Postgres` not covered
    |

https://github.com/prisma/prisma-engines/actions/runs/12814975691/job/35732637858?pr=5129

@jacek-prisma
Copy link
Contributor Author

jacek-prisma commented Jan 17, 2025 •
edited
Loading

@aqrln this is the base branch failing (compiler), I think my branch is fine: https://github.com/prisma/prisma-engines/actions/runs/12814975691/job/35732639217?pr=5129. The job builds both to compare sizes.

@aqrln
Copy link
Member

aqrln commented Jan 17, 2025

@aqrln this is the base branch failing (compiler), I think my branch is fine: prisma/prisma-engines/actions/runs/12814975691/job/35732639217?pr=5129. The job builds both to compare sizes.

ah gotcha

aqrln
aqrln reviewed Jan 17, 2025
View reviewed changes
query-engine/query-engine-node-api/src/engine.rs Outdated
@@ -354,50 +354,6 @@ impl QueryEngine {
.await
}

#[napi]
pub async fn compile(&self, request: String, human_readable: bool) -> napi::Result<String> {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, could we keep this method for now until we migrate the client side to use the new library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will add it back

aqrln
aqrln approved these changes Jan 17, 2025
View reviewed changes
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.

Nice work!

.github/workflows/formatting.yml Outdated
Comment on lines 24 to 25
env:
RUSTFLAGS: "-Dwarnings"
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@jacek-prisma jacek-prisma merged commit 48e2fc1 into compiler Jan 17, 2025
362 of 363 checks passed
@jacek-prisma jacek-prisma deleted the chore/query-compiler-crate branch January 17, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aqrln aqrln aqrln approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jacek-prisma @aqrln