-
Notifications
You must be signed in to change notification settings - Fork 238
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: resolve relations with lateral joins #4509
Conversation
CodSpeed Performance ReportMerging #4509 will degrade performances by 8.26%Falling back to comparing Summary
Benchmarks breakdown
|
dbe778b
to
a84fe43
Compare
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 PR is big enough and outside of my full area of knowledge as for me alone not being able to provide a meaningful review. For that, @Weakky kindly walked me through the code changes and we discussed some of the decisions made.
The conclusion, after that conversation is that the behavior is correct (as expressed by the tests) and that there is no reason to block shipping the Lateral Join strategy for relation fetching as a preview feature.
match ctx.max_bind_values { | ||
Some(chunk_size) if query_arguments.should_batch(chunk_size) => { | ||
return Err(SqlError::QueryParameterLimitExceeded( | ||
"Joined queries cannot be split into multiple queries just yet. If you encounter this error, please open an issue" |
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.
We discussed how this could trigger by a user exceeding the number of bind params artificially (f.i. by dynamically composing wrong a query) and we agreed this wouldn't be subject of an issue on our end. As such this message can be misleading and frame as if we did something wrong with the implementation. A more appropriate tactic might be ignoring batching altogether and letting the code reach the DB, bubbling up to the user any potential query parameter limit exceeded.
The build script had an invalid `sed` command with an extra `''` argument that caused it to fail with ``` sed: can't read s/name = "query_engine_wasm"/name = "query_engine"/g: No such file or directory ``` This is reproducible both on CI and locally for me. Perhaps it was written for BSD sed and doesn't work with GNU sed (so it always fails on Linux and also fails on macOS inside prisma-engines Nix flake but maybe it works on macOS without Nix)? Because of this, a broken package was published from CI. The commit fixes the `sed` command and adds `set -e` so that errors like this would fail CI instead of silently continuing and doing wrong things.
Overview
This PR enables the QueryEngine to resolve relations using LATERAL JOINs. This feature is currently only available on Postgres & CockroachDB and under the
relationJoins
preview feature flags. To minimize damage, we have, as much as possible, created new code paths for this new feature.To minimize data duplicates traditionally caused by joins, we're using JSON aggregations. Here's what a query may look like on a one2m relation:
This is roughly achieved thanks to the addition/update of some core abstractions:
FieldSelection
: ARelation
variant is added. Previously, relations were always collected as separateReadQuery
. Please note that, for now, separate read queries are still collected even whenrelationJoins
is enabled.RelationLoadStrategy { Query, Join }
: A new enum that let's the core and the connectors decide how to load relations. While I would very much like to keep the way relations are loaded entirely to the connectors' responsibility, we, unfortunately, have to handle results differently. So for now, the core passes down to the connectors how to resolve relations, and the result is handled according to that strategy.RecordSelectionWithRelation
: A new type ofRecordSelection
(eventually passed to the serializer) was added to serialize data differently (given that relations are now part of theManyRecords
when joins are used, as opposed to before where relations data were independently stored as results of nestedRelatedRecordsQuery
)