-
Notifications
You must be signed in to change notification settings - Fork 235
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
fix(qe): make UpdateRecord::WithSelection
nodes return RecordSelection
#4508
fix(qe): make UpdateRecord::WithSelection
nodes return RecordSelection
#4508
Conversation
CodSpeed Performance ReportMerging #4508 will not alter performanceComparing Summary
|
- Change `FieldSelection::matches` to mean that the current field selection is a subset of `SelectionResult` instead of a superset. - Project the selected tuple into the expected cardinality.
`SelectionResult::new` is not generic, compiler already knows the type.
This reverts commit d56319a.
…ion" This reverts commit 6ee982b.
d5731c7
to
94c60ce
Compare
Even though there technically should be a need for name if the node's results don't need to be serialized and only fulfill the requirements of another node, we still need some name to build `RecordSelection`. Previously the update node with selection that has no name would return a `QueryResult::Id` instead of `QueryResult::RecordSelection` to work around that, but it caused problems as the query interpreter relied on `QueryResult::Id` being the primary identifier and not arbitrary fields. This commit makes the name required for `UpdateRecord::WithSelection` and provides a dummy name when the node doesn't directly correspond to user's query. This is consistent with other intermediate nodes that need to build `RecordSelection` and use dummy names (like `reload` or `find_children_by_parent`) or empty strings.
UpdateRecord::WithSelection
nodes return RecordSelection
@@ -200,7 +200,7 @@ where | |||
|
|||
Query::Write(WriteQuery::UpdateRecord(UpdateRecord::WithSelection( | |||
UpdateRecordWithSelection { | |||
name: Some(field.name.to_owned()), |
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.
Removed to_owned()
because field.name
is already an owned string, and we don't need it anymore here so we can just move it without cloning.
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.
Changes look good, and the reproduction looks solid
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.
Nice job 👍
Query graph nodes that are not result nodes and fulfill the requirements of other nodes don't inherently need names because their results aren't serialized. However, we still need some name to be able to build
RecordSelection
because it's required there. Previously the update node with selection that has no name would return aQueryResult::Id
instead ofQueryResult::RecordSelection
to work around that, but it caused problems as the query interpreter relied onQueryResult::Id
being the primary identifier and not arbitrary fields.This PR makes the name required for
UpdateRecord::WithSelection
and provides a stub name when the node doesn't directly correspond to the client's query. This is consistent with other intermediate nodes that need to buildRecordSelection
. Some of them use constant placeholder names (likefind_children_by_parent
forRelatedRecordsQuery
), some of them use empty strings (e.g.CreateRecord
does this).I chose an empty string because it needs to be owned here and an empty
String::new()
doesn't allocate memory, and the specific value doesn't matter anyway because there's likely not even a way to observe this name (it's not used inDisplay
andToGraphviz
impls forUpdateRecord
).Fixes: prisma/prisma#21182