fix: batching should work with non unique scalar filters #3328

Weakky · 2022-10-25T10:22:50Z

Overview

With the extension of where unique inputs, which now allow non-unique filters, the batching logic was broken. Here are the new rules:

A batched query is compacted, only if:

the findUnique queries only deal with EQUALS scalar filters
- Both syntaxes are supported: { field: <val> } or { field: { equals: <val> } }
- That includes compound filters as well: { field1_field2: { field1: <val>, field2: <val> } }

If the condition above is not fulfilled, then we fall back to a simple batch, which executes all findUnique queries separately.

Concretely, this means:

All “previous” queries (as-in, queries that don’t use the extended where unique) will still be supported
Queries using extended where which only use equal scalar filters (of the same model) will now be supported as well (eg: the issue above which simply adds { deletedAt: null }
As soon as a boolean operator, a relation filter or any scalar filter that's not using equal is present in the where arg, queries will be processed individually instead of being compacted.

Weakky · 2022-10-25T12:18:43Z

query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_5952.rs

-            r#"query {findUniqueArtist(where:{firstName_lastName_birth:{firstName:"Michael",netWorth:"236600000.12409"}}) {firstName netWorth}}"#.to_string(),
-            r#"query {findUniqueArtist(where:{firstName_lastName_birth:{firstName:"George",netWorth:"-0.23660010012409"}}) {firstName netWorth}}"#.to_string(),
+            r#"query {findUniqueArtist(where:{firstName_netWorth:{firstName:"Michael",netWorth:"236600000.12409"}}) {firstName netWorth}}"#.to_string(),
+            r#"query {findUniqueArtist(where:{firstName_netWorth:{firstName:"George",netWorth:"-0.23660010012409"}}) {firstName netWorth}}"#.to_string(),


This test was added by #3210. It started failing in my PR. I'm not sure how it's ever been able to work. The compound field name was clearly wrong since day 1, I don't get it 😅...

Ohhh, I get it now. The previous algorithm would not validate the fields against the schema. It would simply check if any of the filters were objects and if so, assume it's a compound unique (because no other kind of filters were possible). In this case, it would take every nested filter individually to put it back in a findMany query. eg:

before:

findUnique(where: { firstName_lastName_bith: { firstName: "A", netWorth: 12 } })

after:

findMany(where: { firstName: "A", netWorth: 12 })

This essentially means the compound unique field name was never sent to the schema validation, which is why it worked.

With this new PR though, we use the query schema to find out what kind of filters we're dealing with more safely.

Hence why it's now failing, because it cannot find firstName_lastName_birth in the query schema as a scalar or compound unique field, so it assumes we cannot compact the queries (because it thinks it's a relation filter), so it sends both queries individually and the schema validation kicks in, saying that firstName_lastName_birth doesn't exist.

Shows the new logic is safer, that's cool.

Weakky · 2022-10-25T12:39:01Z

query-engine/core/src/response_ir/mod.rs

+            Self::List(list) => {
+                let mut values = vec![];
+
+                for item in list {
+                    if let Some(pv) = item.into_value() {
+                        values.push(pv)
+                    } else {
+                        return None;
+                    }
+                }
+
+                Some(PrismaValue::List(values))
+            }


This is needed for 👇, line 55.

prisma-engines/query-engine/core/src/response_ir/mod.rs

Lines 47 to 63 in eaaec29

pub fn index_by(self, keys: &[String]) -> Vec<(Vec<QueryValue>, Map)> {

let mut map = Vec::with_capacity(self.len());

for item in self.into_iter() {

let inner = item.into_map().unwrap();

let key: Vec<QueryValue> = keys

.iter()

.map(|key| inner.get(key).unwrap().clone().into_value().unwrap())

.map(QueryValue::from)

.collect();

map.push((key, inner));

}

map

}

which is used to map the findMany result back to the findUniques.

One of the new tests showed that scalar lists were failing because it couldn't convert serialized scalar lists back to prisma values.

This piece of code handles that case.

Thanks for the clarification, makes sense!

garrensmith

nice fix

garrensmith · 2022-10-25T13:59:56Z

query-engine/core/src/query_document/mod.rs

+    /// Invalid filters are:
+    /// - non scalar filters (ie: relation filters, boolean operators...)
+    /// - any scalar filters that is not `EQUALS`
+    fn operation_contain_invalid_filter(op: &Operation, schema: &QuerySchemaRef) -> bool {


The name for this function isn't quite right. I think it needs to be something like invalid_compact_filter or can_use_filter_with_compact

miguelff

This was a great learning opportunity, code looks good 👍, I only left some actionable feedback in one of the comments about reordering instructions.

miguelff · 2022-10-27T11:28:39Z

query-engine/core/src/query_document/mod.rs

+    /// Returns `true` if the batch document is [`Compact`].
+    #[must_use]
+    pub fn is_compact(&self) -> bool {
+        matches!(self, Self::Compact(..))
+    }


Out of curiosity, nothing actionable.

As far as I can see, this function is used in tests. Is there a reason why we prefer to define these type of helpers in the code rather than tests, when they use public modules like BatchDocument::Compact?

Tbh, there's no real reason. It's true that I could've used assert!(matches!(doc, BatchDocument::Compact(_))). On the other hand, I don't feel like leaking this kind of this API should ever hurt us. If you think otherwise, please let me know

miguelff · 2022-10-27T11:30:25Z

query-engine/core/src/query_document/mod.rs

+                    // we do not compact the queries.
+                    let has_invalid_filters = operations
+                        .iter()
+                        .any(|op| Self::operation_contain_invalid_filter(op, schema));


To avoid unnecessary computation, would it make sense to move this above has_identical_selection_sets return false if it has invalid filters?

miguelff · 2022-10-27T11:32:32Z

query-engine/core/src/query_document/mod.rs

+/// Takes in a unique filter, extract the scalar filters and return a simple list of field/filter.
+/// This list is used to build a findMany query from multiple findUnique queries.
+/// Therefore, compound unique filters walked and each individual field is added. eg:
+/// { field1_field2: { field1: 1, field2: 2 } } -> [(field1, 1), (field2, 2)]
+/// This is because findMany filters don't have the special compound unique syntax.
+///
+/// Furthermore, this list is used to match the results of the findMany query back to the original findUnique queries.
+/// Because of that, we only extract EQUALS filters or else we would have to manually implement other filters.
+/// This is a limitation that _could_ technically be lifted but that's not worth it for now.
+fn extract_filter(where_obj: IndexMap<String, QueryValue>, model: &ModelRef) -> Vec<(String, QueryValue)> {


Thanks for adding good documentation! ✨ 🎩

miguelff · 2022-10-27T11:33:16Z

query-engine/core/src/response_ir/mod.rs

+            Self::List(list) => {
+                let mut values = vec![];
+
+                for item in list {
+                    if let Some(pv) = item.into_value() {
+                        values.push(pv)
+                    } else {
+                        return None;
+                    }
+                }
+
+                Some(PrismaValue::List(values))
+            }


Thanks for the clarification, makes sense!

Weakky commented Oct 25, 2022

View reviewed changes

Weakky added this to the 4.6.0 milestone Oct 25, 2022

Weakky commented Oct 25, 2022

View reviewed changes

garrensmith approved these changes Oct 25, 2022

View reviewed changes

miguelff approved these changes Oct 27, 2022

View reviewed changes

Weakky mentioned this pull request Oct 28, 2022

Preview feature feedback: Extended where unique prisma/prisma#15837

Closed

Weakky added 4 commits November 7, 2022 11:52

fix: batching should work with non unique scalar filters

caf91fb

fix tests

c002862

fix invalid filter logic & add tests

be29027

review fix

f8789c2

Weakky force-pushed the fix/batch-extended-where-unique branch from 585578d to f8789c2 Compare November 7, 2022 10:53

Weakky merged commit 2e719ef into main Nov 7, 2022

Weakky deleted the fix/batch-extended-where-unique branch November 7, 2022 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: batching should work with non unique scalar filters #3328

fix: batching should work with non unique scalar filters #3328

Weakky commented Oct 25, 2022 •

edited

Weakky Oct 25, 2022 •

edited

Weakky Oct 25, 2022 •

edited

Weakky Oct 25, 2022 •

edited

miguelff Oct 27, 2022

garrensmith left a comment

garrensmith Oct 25, 2022

miguelff left a comment

miguelff Oct 27, 2022

Weakky Nov 7, 2022

miguelff Oct 27, 2022

miguelff Oct 27, 2022

miguelff Oct 27, 2022

	pub fn index_by(self, keys: &[String]) -> Vec<(Vec<QueryValue>, Map)> {
	let mut map = Vec::with_capacity(self.len());

	for item in self.into_iter() {
	let inner = item.into_map().unwrap();

	let key: Vec<QueryValue> = keys
	.iter()
	.map(\|key\| inner.get(key).unwrap().clone().into_value().unwrap())
	.map(QueryValue::from)
	.collect();

	map.push((key, inner));
	}

	map
	}

fix: batching should work with non unique scalar filters #3328

fix: batching should work with non unique scalar filters #3328

Conversation

Weakky commented Oct 25, 2022 • edited

Overview

Weakky Oct 25, 2022 • edited

Choose a reason for hiding this comment

Weakky Oct 25, 2022 • edited

Choose a reason for hiding this comment

Weakky Oct 25, 2022 • edited

Choose a reason for hiding this comment

miguelff Oct 27, 2022

Choose a reason for hiding this comment

garrensmith left a comment

Choose a reason for hiding this comment

garrensmith Oct 25, 2022

Choose a reason for hiding this comment

miguelff left a comment

Choose a reason for hiding this comment

miguelff Oct 27, 2022

Choose a reason for hiding this comment

Weakky Nov 7, 2022

Choose a reason for hiding this comment

miguelff Oct 27, 2022

Choose a reason for hiding this comment

miguelff Oct 27, 2022

Choose a reason for hiding this comment

miguelff Oct 27, 2022

Choose a reason for hiding this comment

Weakky commented Oct 25, 2022 •

edited

Weakky Oct 25, 2022 •

edited

Weakky Oct 25, 2022 •

edited

Weakky Oct 25, 2022 •

edited