Bug 23875 - Elasticsearch - When sorting by score we should provide a tiebreaker
Summary: Elasticsearch - When sorting by score we should provide a tiebreaker
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 23089
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-22 14:39 UTC by Nick Clemens (kidclamp)
Modified: 2025-05-05 07:54 UTC (History)
9 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes search results when using Elasticsearch so that: - When searching by relevance (the default) and a generic search (such as "*"), search results are returned with the highest record number first (previously, search results were returned with the lowest record number first) - Editing a record and repeating a generic search doesn't change the sort order (previously editing a record would change the search results order). Changing the sort order of results (such as by author (A-Z) or title) continue to work as expected.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker (3.04 KB, patch)
2019-10-22 14:43 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker (3.08 KB, patch)
2019-10-25 06:35 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker (3.06 KB, patch)
2020-08-17 11:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker (2.92 KB, patch)
2024-11-07 13:36 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker (2.95 KB, patch)
2025-04-18 15:01 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker (2.99 KB, patch)
2025-04-19 18:28 UTC, David Nind
Details | Diff | Splinter Review
Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker (3.07 KB, patch)
2025-04-22 14:59 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 23875: Fix tests (1.09 KB, patch)
2025-05-05 07:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2019-10-22 14:39:21 UTC
In ES if we sort by 'relevance' we actually don't pass anything to ES and results are ordered by score.

When doing a search limiting only by collection or type, the scores are all the same. This is fine, except that editing any record will then cause a reordering of the results, even though it won't affect the score.

We should provide a tie-breaker in the case of equal sort scores
Comment 1 Nick Clemens (kidclamp) 2019-10-22 14:43:08 UTC Comment hidden (obsolete)
Comment 2 Ere Maijala 2019-10-23 14:08:43 UTC
Should we care that the sorting by local-number.raw isn't numeric?
Comment 3 Nick Clemens (kidclamp) 2019-10-24 10:23:09 UTC
(In reply to Ere Maijala from comment #2)
> Should we care that the sorting by local-number.raw isn't numeric?

In the mappings local-number is set as a string by default - should we change it to a number? I think in practice it will be a number, however, under ES at least I don't see why someone couldn't map local-number to 001 and manage their own control numbers with letters, so maybe string is okay as long as your numbers are built consistently?
Comment 4 Ere Maijala 2019-10-25 06:35:15 UTC Comment hidden (obsolete)
Comment 5 Ere Maijala 2019-10-25 06:41:14 UTC
(In reply to Nick Clemens from comment #3)
> In the mappings local-number is set as a string by default - should we
> change it to a number? I think in practice it will be a number, however,
> under ES at least I don't see why someone couldn't map local-number to 001
> and manage their own control numbers with letters, so maybe string is okay
> as long as your numbers are built consistently?

Changing it to number would be scary, and I don't think this is really an issue. If we want to make sorting consistent, we could always left-pad the biblionumber with zeros, but I don't really like it since it wouldn't match the biblionumber field in MARC exactly. Alternatively we could have a hardcoded mapping to a numeric biblionumber field. Or make the tie-breaker sort field configurable. But since local-number gives us consistent results, I believe this is good enough for all practical purposes.
Comment 6 Katrin Fischer 2019-10-27 14:19:10 UTC
(In reply to Ere Maijala from comment #5)
> (In reply to Nick Clemens from comment #3)
> > In the mappings local-number is set as a string by default - should we
> > change it to a number? I think in practice it will be a number, however,
> > under ES at least I don't see why someone couldn't map local-number to 001
> > and manage their own control numbers with letters, so maybe string is okay
> > as long as your numbers are built consistently?
> 
> Changing it to number would be scary, and I don't think this is really an
> issue. If we want to make sorting consistent, we could always left-pad the
> biblionumber with zeros, but I don't really like it since it wouldn't match
> the biblionumber field in MARC exactly. Alternatively we could have a
> hardcoded mapping to a numeric biblionumber field. Or make the tie-breaker
> sort field configurable. But since local-number gives us consistent results,
> I believe this is good enough for all practical purposes.

I am not sure if I understand the issue compeletely, but why would local-number be scary as a number? local-number is usually the biblionumber, right? And if it is, I am in doubt about what Nick says that you could just remap it to 001 - at least I would expect issues with that.

I think being able to sort numerical on biblionumber is important so you can use it as a tie breaker as mentioned before, when all other criteria are the some. You you can sort from newer to older.

If we don't sort numerical... that meanst 9 999 2 22 1 11 ... something like that?
Comment 7 Ere Maijala 2019-10-28 07:12:58 UTC
(In reply to Katrin Fischer from comment #6)
> I am not sure if I understand the issue compeletely, but why would
> local-number be scary as a number? local-number is usually the biblionumber,
> right? And if it is, I am in doubt about what Nick says that you could just
> remap it to 001 - at least I would expect issues with that.

Yes, it's usually biblionumber, but there's nothing preventing one from e.g. adding other fields in it too. Unless we hard-code a field for biblionumber, we can't positively say it's always a number.

> If we don't sort numerical... that meanst 9 999 2 22 1 11 ... something like
> that?

Yes.
Comment 8 Katrin Fischer 2019-10-28 07:15:03 UTC
(In reply to Ere Maijala from comment #7)
> (In reply to Katrin Fischer from comment #6)
> > I am not sure if I understand the issue compeletely, but why would
> > local-number be scary as a number? local-number is usually the biblionumber,
> > right? And if it is, I am in doubt about what Nick says that you could just
> > remap it to 001 - at least I would expect issues with that.
> 
> Yes, it's usually biblionumber, but there's nothing preventing one from e.g.
> adding other fields in it too. Unless we hard-code a field for biblionumber,
> we can't positively say it's always a number.
> 
> > If we don't sort numerical... that meanst 9 999 2 22 1 11 ... something like
> > that?
> 
> Yes.

Maybe we should have an unchangeable index for biblionumber then? The sorting could quickly become an issue. People tend to search very 'broadly' and then the tie breaker would come in quickly. At least that's what we see with Zebra quite often with searches for single words and similar.
Comment 9 Ere Maijala 2019-10-28 07:19:03 UTC
(In reply to Katrin Fischer from comment #8)
> Maybe we should have an unchangeable index for biblionumber then? The
> sorting could quickly become an issue. People tend to search very 'broadly'
> and then the tie breaker would come in quickly. At least that's what we see
> with Zebra quite often with searches for single words and similar.

Using non-numeric sort doesn't really matter for tie-breaker unless you care for the order. I'd actually like us to have a hard-coded biblionumber index field. One could also argue that biblionumber should only be the last resort, and publication date should be the primary. We'll probably end up making it configurable. ;)
Comment 10 Katrin Fischer 2019-10-28 07:25:16 UTC
And by the reasoning that we can't prevent other fields being added... doesn't that apply to all other indexes as well? Would Elastic give you an error when searching on a string in a numerical index or just not sort them 'right'?(In reply to Ere Maijala from comment #9)
> (In reply to Katrin Fischer from comment #8)
> > Maybe we should have an unchangeable index for biblionumber then? The
> > sorting could quickly become an issue. People tend to search very 'broadly'
> > and then the tie breaker would come in quickly. At least that's what we see
> > with Zebra quite often with searches for single words and similar.
> 
> Using non-numeric sort doesn't really matter for tie-breaker unless you care
> for the order. I'd actually like us to have a hard-coded biblionumber index
> field. One could also argue that biblionumber should only be the last
> resort, and publication date should be the primary. We'll probably end up
> making it configurable. ;)

publication date can get quickly problematic for serials... the first date in 008 is usually quite old. Every approach has its own problems ;)

Maybe we can move the discussion of adding a hardcoded biblionumber index into another bug, but it really bugs me that the sorting of a clearly numerical value would be alphabetic... it just seems very wrong. I've ended up explaining the 'last resort' ordering a lot of times to libraries.
Comment 11 Ere Maijala 2019-10-28 08:02:19 UTC
(In reply to Katrin Fischer from comment #10)
> publication date can get quickly problematic for serials... the first date
> in 008 is usually quite old. Every approach has its own problems ;)

True, and I didn't mean to imply that should be a default. :)

> Maybe we can move the discussion of adding a hardcoded biblionumber index
> into another bug, but it really bugs me that the sorting of a clearly
> numerical value would be alphabetic... it just seems very wrong. I've ended
> up explaining the 'last resort' ordering a lot of times to libraries.

I'm fine either way. But clearly at least a follow-up to this bug is needed.
Comment 12 Martin Renvoize (ashimema) 2020-03-11 14:10:48 UTC
Are you intending on continuing here with the requested followup Nick?

I think the suggestion was to have a fixed biblionumber index which is numeric and use that for the tie-breaker by default.. as aposed to using the existing 'local-number' index which can be re-mapped?
Comment 13 Katrin Fischer 2020-08-16 09:36:10 UTC
Patch no longer applies, please rebase!

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker
error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-23875-Explicitly-srt-searches-by-score-and-pro-n_vYDq.patch
Comment 14 Nick Clemens (kidclamp) 2020-08-17 11:14:53 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2020-08-22 22:06:24 UTC
(In reply to Martin Renvoize from comment #12)
> Are you intending on continuing here with the requested followup Nick?
> 
> I think the suggestion was to have a fixed biblionumber index which is
> numeric and use that for the tie-breaker by default.. as aposed to using the
> existing 'local-number' index which can be re-mapped?

I really believe we should do that and have a numerical sort on the biblionumber as a tiebreaker. 

I hear your argument about remapping local-number, but I believe that this is a concern that needs more work and thinking overall if we want to officially support it. At the moment, doing that, would probably break your Zebra pretty badly and now allow to switch back and forth easily anymore.

Having a numerical sort on biblionumber on the tiebreaker would also mimick Zebra a bit allowing to compare relevance sort results more easily.
Comment 16 Fridolin Somers 2022-01-25 10:41:39 UTC
What a strange bug.
Any updates ?
Comment 17 Fridolin Somers 2023-10-02 18:34:27 UTC
30879
Comment 18 Fridolin Somers 2023-10-02 18:35:39 UTC
Ah I had issues with local-number sort, found Bug 30879
Comment 19 Katrin Fischer 2023-12-09 12:06:18 UTC
I believe we have solved the numerical sorting by now - could this be revived?
Comment 20 Fridolin Somers 2024-02-08 16:03:34 UTC
isnt it possible to sort on '_id' (ES document id) instead of local-number ?
Comment 21 Fridolin Somers 2024-02-08 16:05:28 UTC
(In reply to Fridolin Somers from comment #20)
> isnt it possible to sort on '_id' (ES document id) instead of local-number ?

Ah there seems to be a performance issue with sorting on '_id', bad idea ;)
Comment 22 Nick Clemens (kidclamp) 2024-11-07 13:36:12 UTC Comment hidden (obsolete)
Comment 23 Nick Clemens (kidclamp) 2024-11-07 13:38:01 UTC
We turned local-number into a number on bug 30879, can we move this one again?
Comment 24 David Nind 2025-01-02 21:02:38 UTC
The patch no longer applies 8-(...

git bz apply 23875

Bug 23875 - Elasticsearch - When sorting by score we should provide a tiebreaker

174227 - Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker
Using index info to reconstruct a base tree...
M	Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
M	t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
Auto-merging Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23875: Explicitly srt searches by score and provide local-number as tie breaker
Comment 25 Nick Clemens (kidclamp) 2025-04-18 15:01:28 UTC Comment hidden (obsolete)
Comment 26 David Nind 2025-04-19 18:28:34 UTC Comment hidden (obsolete)
Comment 27 David Nind 2025-04-19 18:38:44 UTC
I've attempted a release not, but not really sure I got this right!
Comment 28 Martin Renvoize (ashimema) 2025-04-22 14:59:51 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2025-04-23 05:19:46 UTC
Sounds like a good improvement, thanks!
Comment 30 Katrin Fischer 2025-04-23 07:19:21 UTC
Fixing authorship to avoid another mailmap:
From: Nick <nick@bywatersolutions.com>
Comment 31 Katrin Fischer 2025-04-23 07:42:44 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 32 Jonathan Druart 2025-04-23 09:13:55 UTC
Koha_Main/3227 is failing

10:42:56 koha_1       | [Request] ** [http://es:9200]-[400] [query_shard_exception] No mapping found for [local-number] in order to sort on, with: {"index":"koha_kohadev_mydb","index_uuid":"Gof8vi8fTLy1hzF1wmgyDw"}, called from sub Search::Elasticsearch::Role::Client::Direct::__ANON__ at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch/Search.pm line 98. With vars: {'body' => {'error' => {'type' => 'search_phase_execution_exception','failed_shards' => [{'node' => '9YF9t7UURligLeU9lgwFog','index' => 'koha_kohadev_mydb','shard' => 0,'reason' => {'index_uuid' => 'Gof8vi8fTLy1hzF1wmgyDw','reason' => 'No mapping found for [local-number] in order to sort on','type' => 'query_shard_exception','index' => 'koha_kohadev_mydb'}}],'phase' => 'query','root_cause' => [{'type' => 'query_shard_exception','index' => 'koha_kohadev_mydb','index_uuid' => 'Gof8vi8fTLy1hzF1wmgyDw','reason' => 'No mapping found for [local-number] in order to sort on'}],'reason' => 'all shards failed','grouped' => bless( do{\(my $o = 1)}, 'JSON::PP::Boolean' )},'status' => 400},'request' => {'mime_type' => 'application/json','path' => '/koha_kohadev_mydb/_search','ignore' => [],'method' => 'GET','serialize' => 'std','body' => {'size' => 20,'sort' => [{'_score' => {'order' => 'desc'}},{'local-number' => {'order' => 'desc'}}],'aggregations' => {'su-geo' => {'terms' => {'size' => '20','field' => 'su-geo__facet'}},'ln' => {'terms' => {'field' => 'ln__facet','size' => '20'}},'ccode' => {'terms' => {'field' => 'ccode__facet','size' => '20'}},'location' => {'terms' => {'size' => '20','field' => 'location__facet'}},'author' => {'terms' => {'size' => '20','field' => 'author__facet'}},'title-series' => {'terms' => {'size' => '20','field' => 'title-series__facet'}},'itype' => {'terms' => {'size' => '20','field' => 'itype__facet'}},'holdingbranch' => {'terms' => {'size' => '20','field' => 'holdingbranch__facet'}},'subject' => {'terms' => {'field' => 'subject__facet','size' => '20'}},'homebranch' => {'terms' => {'size' => '20','field' => 'homebranch__facet'}}},'from' => 0,'query' => {'query_string' => {'query' => 'easy','analyze_wildcard' => $VAR1->{'body'}{'error'}{'grouped'},'fields' => [],'fuzziness' => 'auto','lenient' => $VAR1->{'body'}{'error'}{'grouped'},'default_operator' => 'AND','type' => 'cross_fields'}}},'qs' => {'track_total_hits' => 'true'}},'status_code' => 400}
10:42:56 koha_1       | Unable to perform your search. Please try again.
10:42:56 koha_1       | # Looks like your test exited with 255 just after 4.
10:42:56 koha_1       | [08:42:01] t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t
Comment 33 Katrin Fischer 2025-04-23 11:07:04 UTC
Thanks, Joubu. Nick, can we get a quick fix? Let us know if we can help (lots of patches to push...!)
Comment 34 Jonathan Druart 2025-05-05 07:50:40 UTC
Created attachment 181892 [details] [review]
Bug 23875: Fix tests
Comment 35 Jonathan Druart 2025-05-05 07:54:36 UTC
(In reply to Jonathan Druart from comment #34)
> Created attachment 181892 [details] [review] [review]
> Bug 23875: Fix tests

Patch pushed.