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: generate m2m connects and disconnects in the compiler #5153

Merged
merged 9 commits into from
Feb 7, 2025

Conversation

jacek-prisma
Copy link
Contributor

@jacek-prisma jacek-prisma commented Feb 6, 2025

Closes ORM-561.

@jacek-prisma jacek-prisma requested a review from a team as a code owner February 6, 2025 12:08
@jacek-prisma jacek-prisma requested review from FGoessler and removed request for a team February 6, 2025 12:08
Copy link
Contributor

github-actions bot commented Feb 6, 2025

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.120MiB 2.119MiB 1.102KiB
Postgres (gzip) 848.879KiB 848.356KiB 536.000B
Mysql 2.082MiB 2.081MiB 1.102KiB
Mysql (gzip) 834.955KiB 833.994KiB 984.000B
Sqlite 1.992MiB 1.990MiB 1.102KiB
Sqlite (gzip) 798.112KiB 797.680KiB 442.000B

Copy link

codspeed-hq bot commented Feb 6, 2025

CodSpeed Performance Report

Merging #5153 will not alter performance

Comparing feat/compiler-connect-m2m (29d5484) with main (744af5d)

Summary

✅ 11 untouched benchmarks

@jacek-prisma jacek-prisma force-pushed the feat/compiler-connect-m2m branch from 3f22a25 to 5aecb49 Compare February 6, 2025 17:01
@aqrln aqrln added this to the 6.4.0 milestone Feb 6, 2025
expr: Box::new(expr),
})
} else {
Ok(expr)
}
}
}

fn generate_projected_dependency_name(source: NodeRef, field: &SelectedField) -> String {
format!("{}${}", source.id(), field.prisma_name())
Copy link
Member

Choose a reason for hiding this comment

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

even when we have just one edge, doing this instead of shadowing makes the query plan much nicer to read 👍

Comment on lines 217 to 218
// TODO: we ignore chunking for now
let linking_scalars = field.related_field().left_scalars();
Copy link
Member

Choose a reason for hiding this comment

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

Could we move it inside the if let statement below?

Copy link
Contributor Author

@jacek-prisma jacek-prisma Feb 7, 2025

Choose a reason for hiding this comment

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

noticed that field.related_field().left_scalars() was being called twice in this function so I extracted it into a single call instead

expression: pretty
input_file: query-compiler/query-compiler/tests/data/create-m2m.json
---
let 0 = unique (query «INSERT INTO "public"."User" ("email") VALUES ($1)
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR but I'm only now realizing we're missing the check if the query graph is transactional, and the corresponding query plan node. Should be easy to add.

in execute «INSERT INTO "public"."_CategoryToPost" ("B","A") VALUES
($1,$2) ON CONFLICT DO NOTHING»
params [var(1$id as Int), var(2$id as Int)];
let 4 = let 0$id = mapField id (get 0)
Copy link
Member

Choose a reason for hiding this comment

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

[unrelated to this PR] We should later introduce optimization passes and, among them, eliminate duplicate mapField expressions (here we even already have it in scope). Although they are cheap in this case (since 0 is unique), in general we may be mapping a long list.

= $1 OFFSET $2»
params [var(@parent$id as Int),
const(BigInt(0))]) on left.(id) = right.(userId) as posts
in get 4
Copy link
Member

Choose a reason for hiding this comment

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

[unrelated to this PR] rewriting let x = y in get x to y is also an obvious candidate for optimization, although here it's mostly cosmetic

Comment on lines 242 to 247
)) => child_column.in_selection(val.clone().decorate(
Some("prisma-comma-repeatable-start"),
Some("prisma-comma-repeatable-end"),
)),
_ => child_column.in_selection(values),
};
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this special handling for ValueType::Var in in_selection should be in quaint itself and not on this level

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 requested a review from aqrln February 7, 2025 12:12
.so_that(parent_id_criteria.and(child_id_criteria))
.add_traceparent(self.context.traceparent);

let query = write::delete_relation_table_records(&field, parent_id, child_ids, &self.context);
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@jacek-prisma jacek-prisma merged commit 7eac4c9 into main Feb 7, 2025
367 checks passed
@jacek-prisma jacek-prisma deleted the feat/compiler-connect-m2m branch February 7, 2025 13:38
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