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.

Sign up for GitHub

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

Jump to bottom

Fixes for group-by #1938

Merged
merged 25 commits into from
May 22, 2023
Merged

Fixes for group-by #1938

merged 25 commits into from
May 22, 2023

Conversation

generall
Copy link
Member

  • Fix for the payload selector:

    • search request doesn't support nested payload selectors, but group_by should
    • New logic only pre-selects top-level key, which might be a subject for possible further improvements

  • additional must-not condition for re-search requests doesn't work with multiple payload values per field. It excludes too much

    • had to introduce a new except match condition, which fixes the problem.
    • as a bonus: except condition also available outside the groups
    • test for reproducing the problem also included

@generall generall requested review from agourlay and coszio May 21, 2023 19:12
timvisee
timvisee approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@timvisee timvisee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

I do not see any logical issues. Added some Rust idiomaticness comments. Approved anyway as these are not required.

lib/collection/src/grouping/group_by.rs Outdated Show resolved Hide resolved
lib/collection/src/grouping/group_by.rs Outdated Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Outdated Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Outdated Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Outdated Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Outdated Show resolved Hide resolved
lib/segment/src/index/query_optimization/condition_converter.rs Outdated Show resolved Hide resolved
lib/segment/src/index/query_optimization/condition_converter.rs Outdated Show resolved Hide resolved
lib/segment/src/index/query_optimization/condition_converter.rs Outdated Show resolved Hide resolved
lib/segment/src/vector_storage/mod.rs Outdated Show resolved Hide resolved
timvisee
timvisee requested changes May 22, 2023
View reviewed changes
Copy link
Member

@timvisee timvisee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did have a possible logic issue, see this. I'll re-approved once this is fixed.

coszio
coszio previously approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@coszio coszio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix to add the except match. Though can it be that the estimation for must_not can be improved in the future?

lib/collection/src/grouping/group_by.rs Outdated Show resolved Hide resolved
lib/collection/src/grouping/group_by.rs Outdated Show resolved Hide resolved
lib/collection/src/grouping/group_by.rs Outdated Show resolved Hide resolved
lib/segment/src/index/field_index/full_text_index/text_index.rs Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Outdated Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Outdated Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Show resolved Hide resolved
lib/segment/src/index/field_index/map_index.rs Show resolved Hide resolved
@coszio coszio dismissed their stale review May 22, 2023 13:32

Meant to request the change of except_on and match_on

@coszio
Copy link
Contributor

coszio commented May 22, 2023

@timvisee requested changes are fixed now 👍

@generall generall requested review from timvisee and coszio May 22, 2023 17:51
@timvisee timvisee self-assigned this May 22, 2023
generall and others added 12 commits May 22, 2023 20:11
@generall
except cardinality estimation
a6ee744
@generall
implement match except iterator and api
97f9b54
@generall
use except instead of must-not + test
a2efb9c
@timvisee @generall
Fix doc error
827a146
@generall @timvisee
Update lib/collection/src/grouping/group_by.rs
510c9b9 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @timvisee
Update lib/segment/src/index/field_index/map_index.rs
419705d 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @timvisee
Update lib/segment/src/index/field_index/map_index.rs
f1c4dd9 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @timvisee
Update lib/segment/src/index/field_index/map_index.rs
57df55b 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @timvisee
Update lib/segment/src/index/query_optimization/condition_converter.rs
2843a84 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @timvisee
Update lib/segment/src/index/query_optimization/condition_converter.rs
5f0e7ab 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @timvisee
Update lib/segment/src/index/query_optimization/condition_converter.rs
eed919a 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @timvisee
Update lib/segment/src/vector_storage/mod.rs
42531ed 
Co-authored-by: Tim Visée <tim+github@visee.me>
generall and others added 11 commits May 22, 2023 20:11
@generall @timvisee
Update lib/segment/src/index/field_index/map_index.rs
6b9b667 
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall @agourlay
Update lib/collection/src/grouping/group_by.rs
6a34d8a 
Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
@generall @agourlay
Update lib/segment/src/index/field_index/map_index.rs
7b7a5e1 
Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
@generall @coszio
Update lib/segment/src/index/field_index/map_index.rs [skip ci]
a777540 
Co-authored-by: Luis Cossío <luis.cossio@qdrant.com>
@coszio @generall
fix: except_on and match_on now produce Vec<Condition>s
7be2c73
@coszio @generall
Apply suggestions from code review (lib/segment/src/index/field_index…
0f31a1b 
…/map_index.rs)
@coszio @generall
fix: reset review suggestion
b7360d8
@timvisee @generall
Remove unnecessary move
9dd582b
@timvisee @generall
Use Rust idiomatic map_else rather than match-none-false
7d0538b
@generall
is-null -> is-empty
e69db71
@coszio @generall
de-comment drop_collection
a65ee28
@timvisee timvisee removed their assignment May 22, 2023
timvisee
timvisee approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@timvisee timvisee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 😄

@generall generall merged commit d624512 into dev May 22, 2023
generall added a commit that referenced this pull request May 22, 2023
@generall @timvisee
@agourlay @coszio
Fixes for group-by (#1938)
f5dfeef 
* fix payload seletor

* clippy

* except cardinality estimation

* implement match except iterator and api

* use except instead of must-not + test

* Fix doc error

* Update lib/collection/src/grouping/group_by.rs

Co-authored-by: Tim Visée <tim+github@visee.me>

* Update lib/segment/src/index/field_index/map_index.rs

Co-authored-by: Tim Visée <tim+github@visee.me>

* Update lib/segment/src/index/field_index/map_index.rs

Co-authored-by: Tim Visée <tim+github@visee.me>

* Update lib/segment/src/index/field_index/map_index.rs

Co-authored-by: Tim Visée <tim+github@visee.me>

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

Co-authored-by: Tim Visée <tim+github@visee.me>

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

Co-authored-by: Tim Visée <tim+github@visee.me>

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

Co-authored-by: Tim Visée <tim+github@visee.me>

* Update lib/segment/src/vector_storage/mod.rs

Co-authored-by: Tim Visée <tim+github@visee.me>

* Update lib/segment/src/index/field_index/map_index.rs

Co-authored-by: Tim Visée <tim+github@visee.me>

* Update lib/collection/src/grouping/group_by.rs

Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>

* Update lib/segment/src/index/field_index/map_index.rs

Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>

* Update lib/segment/src/index/field_index/map_index.rs [skip ci]

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

* fix: `except_on` and `match_on` now produce `Vec<Condition>`s

* Apply suggestions from code review (lib/segment/src/index/field_index/map_index.rs)

* fix: reset review suggestion

* Remove unnecessary move

* Use Rust idiomatic map_else rather than match-none-false

* is-null -> is-empty

* de-comment drop_collection

---------

Co-authored-by: timvisee <tim@visee.me>
Co-authored-by: Tim Visée <tim+github@visee.me>
Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
Co-authored-by: Luis Cossío <luis.cossio@qdrant.com>
Co-authored-by: Luis Cossío <luis.cossio@outlook.com>
@agourlay agourlay deleted the match-except branch July 12, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agourlay agourlay agourlay left review comments

@coszio coszio coszio approved these changes

@timvisee timvisee timvisee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@generall @coszio @agourlay @timvisee