-
Notifications
You must be signed in to change notification settings - Fork 250
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
qe: partially refactor relation aggregation selection #4658
Conversation
WASM Size
|
CodSpeed Performance ReportMerging #4658 will not alter performanceComparing Summary
|
🚀 WASM query-engine performance will improve by 5.33%Full benchmark report
After changes in ef0e83b |
58b6759
to
d837906
Compare
c5c9ca4
to
26c6166
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.
(Reviewer's note)
Most diff in this PR is about getting rid of aggr_selections
(whether as a variable or as a function parameter), or replacing it with virtual_selections
.
The PR, despite its size, seems cohesive and very useful. I'll leave the final comment to Flavian though, as I don't fully grasp the context of these changes.
26c6166
to
74b3e09
Compare
query-engine/connectors/sql-query-connector/src/database/operations/read.rs
Show resolved
Hide resolved
`id` was returned instead of `_id` because of the wrong method being called. This bug was previously masked by serialization logic silently filtering out unknown fields before `.zip()`, making the rest of the fields mismatch the corresponding values, which wasn't a problem in this case because there are no values in an empty result.
74b3e09
to
d96e721
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.
Long due tech debt work (that the team should've done, not that you're late), so much cleaner, nice work 👍
Refactor relation aggregation selections by making them a kind of
SelectedField
. More generally, this introduces virtual fields (i.e. fields that don't exist in the model and represent a computation instead) which relation aggregations (i.e._count
) are one and so far the only kind of.This eliminates code duplication and special cases for relation aggregations in many places:
selected_fields
are the source of truthOne particular special case that is avoidable with the new design but still exists is that we still store the list of virtual selection fields separately in the
RecordSelection
struct. The newvirtual_fields
field plays the role of the oldaggregation_rows
one, except the old one stored extracted aggregation data together with definitions of the fields while the new one stores the pure definitions. It shouldn't be necessary, and bothfields: Vec<String>
andvirtual_fields: Vec<VirtualSelection>
can be replaced with a singlefields: Vec<SelectedField>
. This also implies splittingselected_fields
in query graph nodes intofull_selection
anduser_selection
/serialized_selection
and eliminating separateselection_order
field. These changes were de-scoped from this PR to avoid changing the write operations that depend onRecordSelection
and to timebox the refactoring, and will happen separately in a future PR.Collection of both virtuals and non-virtuals in
collect_selected_fields
in the query graph builder, as well as processing the scalars together with virtuals in the serializer, both happen in a single pass already in anticipation of future changes.WASM benchmarks are about 5% faster on my machine with these changes.
This is preparatory work necessary for making relation aggregation selections work with JOINs (https://github.com/prisma/team-orm/issues/700).
Closes: https://github.com/prisma/team-orm/issues/815