-
Notifications
You must be signed in to change notification settings - Fork 240
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(qe): implement relationLoadStrategy
query argument
#4601
Conversation
WASM Size
|
90cb9fa
to
133f46f
Compare
CodSpeed Performance ReportMerging #4601 will not alter performanceComparing Summary
|
✅ WASM query-engine: no benchmarks have regressedFull benchmark report
After changes in d2f0998 |
Client-side counterpart of <prisma/prisma-engines#4601>. Closes: prisma/team-orm#703
Client-side counterpart of <prisma/prisma-engines#4601>. Closes: prisma/team-orm#703
relationLoadStrategy
query argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, nice work 💯
7d8c992
to
42211f0
Compare
@@ -54,7 +54,7 @@ fi | |||
# Check if the system has engineer installed, if not, use a local copy. | |||
if ! type "engineer" &> /dev/null; then | |||
# Setup Prisma engine build & test tool (engineer). | |||
curl --fail -sSL "https://prisma-engineer.s3-eu-west-1.amazonaws.com/1.65/latest/$OS/engineer.gz" --output engineer.gz | |||
curl --fail -sSL "https://prisma-engineer.s3-eu-west-1.amazonaws.com/1.66/latest/$OS/engineer.gz" --output engineer.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -228,7 +228,7 @@ services: | |||
|
|||
mysql-5-6: | |||
image: mysql:5.6.50 | |||
command: mysqld | |||
command: mysqld --table_definition_cache=2000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, too many tests broke MySQL 5.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed in MySQL 5.7, only 5.1 to 5.6 had this issue.
Client-side counterpart of <prisma/prisma-engines#4601>. Closes: prisma/team-orm#703
@@ -258,6 +259,7 @@ pub(crate) fn get_relation_load_strategy( | |||
ReadQuery::RelatedRecordsQuery(q) => q.has_cursor() || q.has_distinct() || q.has_aggregation_selections(), | |||
_ => false, | |||
}) | |||
&& requested_strategy != Some(RelationLoadStrategy::Query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: (maybe that does not change much), it would be "faster" to check for this condition earlier (maybe in second position?).
The other conditions would not be evaluated then when RelationLoadStrategy is set to query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to tell if it matters without looking at the generated code but yeah it makes sense even from just from stylistic perspective, I think the code reads a bit more naturally with this check earlier. We need to get this merged now as we don't have much time waiting on CI before the release but I can change it in a follow up PR.
Client-side counterpart of <prisma/prisma-engines#4601>. * Ensure `count` operation types are correct by filtering out the `relationLoadStrategy` argument. * Add tests for everything. * Update existing tests. Part of: prisma/team-orm#703 Closes: prisma/team-orm#801
Implement per-query configuration of relation load strategy as described in https://www.notion.so/prismaio/Relation-load-strategy-API-013e3ba957a34d45b4de1b722ca6804d, particularly this section (Variant 1, only top level).
This allows to choose the relation load strategy for all relations in a single query.
Engine API:
Client API:
The new
relationLoadStrategy
argument is available in the following operations:The argument is not available in the following operations:
Tests are included for all operations in this PR.
Additionally, it must not be available in the
count
operation, which needs to be handled separately on the client side, becausecount
is a synthetic operation that does not exist in the query schema and is not known to the engine, and its TS types are generated based onfindMany
args rather thanaggregate
that it translates to.Next steps:
Part of https://github.com/prisma/team-orm/issues/703
Closes: https://github.com/prisma/team-orm/issues/801
Client PR: prisma/prisma#22483