Skip to content

contactsmanager shall limit number of results early - #22057

Merged
MorrisJobke merged 1 commit into
masterfrom
fix/20009/contactsmenu-limit-users
Aug 3, 2020
Merged

contactsmanager shall limit number of results early#22057
MorrisJobke merged 1 commit into
masterfrom
fix/20009/contactsmenu-limit-users

Conversation

@blizzz

Copy link
Copy Markdown
Member

fixes#20009 cf. #20009 (comment)

Optional arguments are added to the ContactsStore API, so it should be okay to backport it to 19. Support for limiting results is implemented down the stack already. Since the contactsmenu reduces results according to sharing.maxAutocompleteResults and the search filter is passed to the backend, limiting the possible results up front does not sacrifice anything, but improves performance.

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

@blizzz
blizzz requested review from ChristophWurst and removed request for ChristophWurstJuly 30, 2020 10:55
@blizzz
blizzzforce-pushed the fix/20009/contactsmenu-limit-users branch from 7828a92 to 646820bCompareJuly 30, 2020 11:02
Comment threadlib/private/Contacts/ContactsMenu/ContactsStore.php Outdated
Comment threadlib/private/Contacts/ContactsMenu/ContactsStore.php Outdated
Comment threadlib/public/Contacts/ContactsMenu/IContactsStore.php
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix/20009/contactsmenu-limit-users branch from 646820b to 7c56283CompareJuly 31, 2020 10:42

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code looks good and makes sense 👍

@faily-bot

This comment has been minimized.

@skjnldsvskjnldsv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🐘

@MorrisJobke
MorrisJobke merged commit 489feca into masterAug 3, 2020
@MorrisJobke
MorrisJobke deleted the fix/20009/contactsmenu-limit-users branch August 3, 2020 13:15
@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable18

@ChristophWurst

ChristophWurst commented Nov 2, 2023

Copy link
Copy Markdown
Member

not sacrifice anything, but improves performance.

The limit is passed down, but not the sort order. So the result correctness is only given if database sorts the same way as the PHP code does later on.

Let's say you have 100 contacts. The database sorts them by UID. You select the first 20 rows, then sort the rows by name. It's not the same result as fetching all 100 contacts, sorting them and then taking the first 20 entries.

SELECT DISTINCT `cp`.`cardid` FROM `oc_cards_properties` `cp` WHERE (`cp`.`addressbookid` = :id) AND ((`cp`.`name` = 'EMAIL') OR (`cp`.`name` = 'FN')) LIMIT 25; is the query run on the DB.

@blizzz

blizzz commented Nov 2, 2023

Copy link
Copy Markdown
MemberAuthor

You have (still) too many results 😅

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ContactsMenu is querying and processing _all_ contacts

6 participants

@blizzz@ChristophWurst@nickvergessen@MorrisJobke@georgehrke@skjnldsv