Skip to content
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 1 commit into from Nov 15, 2022

Conversation

Weakky
Copy link
Member

@Weakky Weakky commented Nov 15, 2022

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:

findUnique(where: { id: 1, name: "Bob" })
findUnique(where: { name: "Bob", id: 1 })

Were transformed to the following list of args:

[
  [("id", 1), ("name", "Bob")],
  [("name", "Bob"), ("id", 1)]
]

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):

[
  ([("id", 1), ("name", "Bob")], <query result>)
]

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 a null (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 HashMaps 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:

#[stable(feature = "rust1", since = "1.0.0")]
impl<K, V, S> PartialEq for HashMap<K, V, S>
where
    K: Eq + Hash,
    V: PartialEq,
    S: BuildHasher,
{
    fn eq(&self, other: &HashMap<K, V, S>) -> bool {
        if self.len() != other.len() {
            return false;
        }

        self.iter().all(|(key, value)| other.get(key).map_or(false, |v| *value == *v))
    }
}

It still remains far more efficient than doing the comparison with two Vecs.

@Weakky Weakky added this to the 4.7.0 milestone Nov 15, 2022
Copy link
Collaborator

@miguelff miguelff left a 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
Copy link
Collaborator

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

@Weakky Weakky merged commit 496dee2 into main Nov 15, 2022
@Weakky Weakky deleted the fix/batch-same-queries branch November 15, 2022 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling findUnique concurrently with different key order causes one of them to return null
3 participants