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: batched queries with same args in diff order #3398
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
garrensmith
approved these changes
Nov 15, 2022
miguelff
approved these changes
Nov 15, 2022
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.
Neat fix. 👏🏾
Comment on lines
+126
to
+148
// At this point, many findUnique queries were converted to a single findMany query and that query was run. | ||
// This means we have a list of results and we need to map each result back to their original findUnique query. | ||
// `data` is the piece of logic that allows us to do that mapping. | ||
// It takes the findMany response and converts it to a map of arguments to result. | ||
// Let's take an example. Given the following batched queries: | ||
// [ | ||
// findUnique(where: { id: 1, name: "Bob" }) { id name age }, | ||
// findUnique(where: { id: 2, name: "Alice" }) { id name age } | ||
// ] | ||
// 1. This gets converted to: findMany(where: { OR: [{ id: 1, name: "Bob" }, { id: 2, name: "Alice" }] }) { id name age } | ||
// 2. Say we get the following result back: [{ id: 1, name: "Bob", age: 18 }, { id: 2, name: "Alice", age: 27 }] | ||
// 3. We know the inputted arguments are ["id", "name"] | ||
// 4. So we go over the result and build the following list: | ||
// [ | ||
// ({ id: 1, name: "Bob" }, { id: 1, name: "Bob", age: 18 }), | ||
// ({ id: 2, name: "Alice" }, { id: 2, name: "Alice", age: 27 }) | ||
// ] | ||
// 5. Now, given the original findUnique queries and that list, we can easily find back which arguments maps to which result | ||
// [ | ||
// findUnique(where: { id: 1, name: "Bob" }) { id name age } -> { id: 1, name: "Bob", age: 18 } | ||
// findUnique(where: { id: 2, name: "Alice" }) { id name age } -> { id: 2, name: "Alice", age: 27 } | ||
// ] | ||
let args_to_results = gql_response |
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 is very much appreciated and explains a bit the reason behind prisma/prisma#16267
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
fixes prisma/prisma#16267
Query arguments were stored as a list of tuple, preventing queries with the same arguments in different order to be compared properly.
For instance, those two queries:
Were transformed to the following list of args:
When building the map of arguments to results from the result of the findMany query though, this is what we used to construct (note: findMany would return a single result since we're batching the same query twice, so there's, expectedly, a single element in that list):
Unfortunately,
[("id", 1), ("name", "Bob")]
is not equal to[("name", "Bob"), ("id", 1)]
, so we'd never find the result for the second query, resulting in anull
(or vice-versa, depending on the order of the selection set of the findMany query).This PR fixes this issue by storing the query arguments as
HashMap
s so that they can be compared regardless of order:{ id: 1, name: "Bob" }
=={ name: "Bob", id: 1 }
.Note that the
PartialEq
impl. for HashMap is not magic and requires iterating over all (k,v) of one of the hashmaps:It still remains far more efficient than doing the comparison with two
Vec
s.