Rewrite nested filters again #1935

generall · 2023-05-20T09:18:26Z

Use regular payload checking function for nested case
Move index-aware checking into the plain payload checker

timvisee

I've commented some minor suggestions to improve code quality a slight bit. You can decide whether you like them or not 😃 .

Not approving yet because there's still one todo!().

lib/segment/src/index/query_optimization/condition_converter.rs

timvisee · 2023-05-22T10:26:55Z

lib/segment/src/payload_storage/nested_query_checker.rs

+    field_indexes: &HashMap<PayloadKeyType, R>,
+) -> BitVec
+where
+    R: AsRef<Vec<FieldIndex>>,


I guess just using &[FieldIndex] would be better here, if possible.

We don't have a situation like with read_to_string where we try to convert multiple types into &FieldIndex. At least, I cannot find it.

lib/segment/src/payload_storage/query_checker.rs

timvisee · 2023-05-22T10:34:49Z

lib/segment/src/payload_storage/query_checker.rs

+            for index in field_indexes.as_ref() {
+                if let Some(index_check_res) = index.check_condition(field_condition, p) {
+                    if index_check_res {
+                        // If at least one object matches the condition, we can return true
+                        return true;
+                    }
+                    index_checked = true;
+                    // If index check of the condition returned something, we don't need to check
+                    // other indexes
+                    break;
+                }
+            }


Different approach without break, not sure which is better:

Suggested change

for index in field_indexes.as_ref() {

if let Some(index_check_res) = index.check_condition(field_condition, p) {

if index_check_res {

// If at least one object matches the condition, we can return true

return true;

}

index_checked = true;

// If index check of the condition returned something, we don't need to check

// other indexes

break;

}

}

if let Some(index_check_res) = field_indexes

.as_ref()

.iter()

.filter_map(|index| index.check_condition(field_condition, p))

.next()

{

// If at least one object matches the condition, we can return true

if index_check_res {

return true;

}

// If index check of the condition returned something, we don't need to check

// other indexes

index_checked = true;

}

An alternative to the alternative is to use find_map() instead of filter_map().next() :P

lib/segment/src/index/query_optimization/condition_converter.rs

agourlay

LGTM!

The new design blows my mind, this is a completely different approach that fits much better in the code base. 🏆

* Add optional ID tracker to check_payload, remove boxed closure * Replace some match with or_else

See: #1935 (comment)

* Replace starts_with/substring with strip_prefix * Transform for-if-let-check-return into any iterator * Transform for-if-return into any iterator * Add comment to describe why check_payload has no ID tracker See: #1935 (comment)

coszio · 2023-05-22T16:17:10Z

lib/segment/src/index/query_optimization/condition_converter.rs

+                    let field_values = payload.get_value(&nested_path).values();
+
+                    for value in field_values {
+                        if let Value::Object(object) = value {
+                            let get_payload = || OwnedPayloadRef::from(object);
+                            if check_payload(
+                                Box::new(get_payload),
+                                // None because has_id in nested is not supported. So retrieving
+                                // IDs throug the tracker would always return None.
+                                None,
+                                &nested.nested.filter,
+                                point_id,
+                                &nested_indexes,
+                            ) {
+                                // If at least one nested object matches, return true
+                                return true;
+                            }
+                        }
+                    }
+                    false


wanted to add this to #1940 but got merged first :)

Suggested change

let field_values = payload.get_value(&nested_path).values();

for value in field_values {

if let Value::Object(object) = value {

let get_payload = || OwnedPayloadRef::from(object);

if check_payload(

Box::new(get_payload),

// None because has_id in nested is not supported. So retrieving

// IDs throug the tracker would always return None.

None,

&nested.nested.filter,

point_id,

&nested_indexes,

) {

// If at least one nested object matches, return true

return true;

}

}

}

false

payload

.get_value(&nested_path)

.values()

.iter()

// Only use objects

.filter_map(|value| value.as_object())

// If at least one nested object matches, return true

.any(|object| {

check_payload(

Box::new(|| OwnedPayloadRef::from(object)),

// None because has_id in nested is not supported. So retrieving

// IDs throug the tracker would always return None.

None,

&nested.nested.filter,

point_id,

&nested_indexes,

)

})

lib/segment/src/payload_storage/query_checker.rs

Co-authored-by: Luis Cossío <luis.cossio@qdrant.com>

* working nested filters * rm unused file * add comment example * todo * remove nester checkers * Box recursive generic Fn types * Box recursive generic Fn types [2/2] * Add optional ID tracker to check_payload, remove boxed closure (#1939) * Add optional ID tracker to check_payload, remove boxed closure * Replace some match with or_else * Some nested filter improvements (#1940) * Replace starts_with/substring with strip_prefix * Transform for-if-let-check-return into any iterator * Transform for-if-return into any iterator * Add comment to describe why check_payload has no ID tracker See: #1935 (comment) * Update lib/segment/src/payload_storage/query_checker.rs Co-authored-by: Luis Cossío <luis.cossio@qdrant.com> * Update lib/segment/src/payload_storage/query_checker.rs Co-authored-by: Luis Cossío <luis.cossio@qdrant.com> * fix clippy --------- Co-authored-by: timvisee <tim@visee.me> Co-authored-by: Tim Visée <tim+github@visee.me> Co-authored-by: Luis Cossío <luis.cossio@qdrant.com>

generall added 2 commits May 20, 2023 11:08

working nested filters

f626a24

rm unused file

88d6c1e

generall requested a review from agourlay May 20, 2023 09:18

agourlay changed the title ~~Rewrive nested filters again~~ Rewrite nested filters again May 22, 2023

generall added 2 commits May 22, 2023 10:42

add comment example

69f2636

todo

8bb9668

timvisee reviewed May 22, 2023

View reviewed changes

lib/segment/src/index/query_optimization/condition_converter.rs Outdated Show resolved Hide resolved

remove nester checkers

70f569c

agourlay reviewed May 22, 2023

View reviewed changes

lib/segment/src/index/query_optimization/condition_converter.rs Outdated Show resolved Hide resolved

timvisee added 2 commits May 22, 2023 15:10

Box recursive generic Fn types

cacae0c

Box recursive generic Fn types [2/2]

1633516

agourlay approved these changes May 22, 2023

View reviewed changes

timvisee mentioned this pull request May 22, 2023

Add optional ID tracker to check_payload, remove boxed closure #1939

Merged

2 tasks

Add optional ID tracker to check_payload, remove boxed closure (#1939)

a930724

* Add optional ID tracker to check_payload, remove boxed closure * Replace some match with or_else

timvisee added a commit that referenced this pull request May 22, 2023

Add comment to describe why check_payload has no ID tracker

fa8ccfe

See: #1935 (comment)

timvisee mentioned this pull request May 22, 2023

Some nested filter improvements #1940

Merged

2 tasks

Some nested filter improvements (#1940)

345f3a7

* Replace starts_with/substring with strip_prefix * Transform for-if-let-check-return into any iterator * Transform for-if-return into any iterator * Add comment to describe why check_payload has no ID tracker See: #1935 (comment)

timvisee approved these changes May 22, 2023

View reviewed changes

coszio reviewed May 22, 2023

View reviewed changes

lib/segment/src/payload_storage/query_checker.rs Outdated Show resolved Hide resolved

coszio reviewed May 22, 2023

View reviewed changes

lib/segment/src/payload_storage/query_checker.rs Outdated Show resolved Hide resolved

generall and others added 3 commits May 22, 2023 18:55

Update lib/segment/src/payload_storage/query_checker.rs

91f586c

Co-authored-by: Luis Cossío <luis.cossio@qdrant.com>

Update lib/segment/src/payload_storage/query_checker.rs

5b593d1

Co-authored-by: Luis Cossío <luis.cossio@qdrant.com>

fix clippy

70c8d40

generall merged commit 60ce060 into dev May 22, 2023

agourlay deleted the retrive-nested-filters-again branch July 12, 2023 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite nested filters again #1935

Rewrite nested filters again #1935

generall commented May 20, 2023

timvisee left a comment

timvisee May 22, 2023

timvisee May 22, 2023

coszio May 22, 2023 •

edited

Loading

agourlay left a comment

coszio May 22, 2023 •

edited

Loading

Rewrite nested filters again #1935

Rewrite nested filters again #1935

Conversation

generall commented May 20, 2023

timvisee left a comment

Choose a reason for hiding this comment

timvisee May 22, 2023

Choose a reason for hiding this comment

timvisee May 22, 2023

Choose a reason for hiding this comment

coszio May 22, 2023 • edited Loading

Choose a reason for hiding this comment

agourlay left a comment

Choose a reason for hiding this comment

coszio May 22, 2023 • edited Loading

Choose a reason for hiding this comment

coszio May 22, 2023 •

edited

Loading

coszio May 22, 2023 •

edited

Loading