Bug 23089

Summary: Elasticsearch - cannot sort on non-text fields
Product: Koha Reporter: Nick Clemens <nick>
Component: Searching - ElasticsearchAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact:
Severity: critical    
Priority: P5 - low CC: alex.arnaud, aspencatteam, e.betemps, ere.maijala, fridolin.somers, glasklas, hayleypelham, jonathan.druart, lucas, martin.renvoize, mkstephens, nicolas.legrand, severine.queune
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00, 19.05.06, 18.11.12
Bug Depends on:    
Bug Blocks: 23875, 23887    
Attachments: Bug 23089: Unit test to highlight sorting issue
Bug 23089: Unit tests
Bug 23089: Fix sorting on sum and regular fields
Bug 23089: Unit tests
Bug 23089: Fix sorting on sum and regular fields
Bug 23089: Fix QueryBuilder tests.
Bug 23089: Fix QueryBuilder tests.
Bug 23089: Unit tests
Bug 23089: Fix sorting on sum and regular fields
Bug 23089: Fix QueryBuilder tests.
Bug 23089: [19.05 and earlier] Use integer field directly for sorting
Bug 23089: [18.11.x] fix tests

Description Nick Clemens 2019-06-10 12:30:37 UTC Comment hidden (obsolete)
Comment 1 Nick Clemens 2019-06-10 12:32:38 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2019-10-10 17:07:12 UTC
Created attachment 93982 [details] [review]
Bug 23089: Unit tests
Comment 3 Nick Clemens 2019-10-10 17:07:15 UTC
Created attachment 93983 [details] [review]
Bug 23089: Fix sorting on sum and regular fields

As of bug 20589 we no longer analyze sort fields and so we no longer need to append ".phrase"
to our sort in searches.

Additionally, sort fields based on 'sum' should also use sum in building the value to sort on

To test:
0 - Be using ES
1 - Find the most circulated item in your collection
2 - Search for '*'
3 - Sort by popularity DESC
4 - Note that item is not first
5 - Try to sort by anything but relevancy, it fails
6 - Apply patch
7 - Redo searches and sorts
8 - Things should now work as expected
Comment 4 Nick Clemens 2019-10-22 14:45:37 UTC
Bumping relevancy, we must fix sorting before release
Comment 5 Ere Maijala 2019-10-23 13:41:55 UTC
Created attachment 94617 [details] [review]
Bug 23089: Unit tests

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 6 Ere Maijala 2019-10-23 13:41:59 UTC
Created attachment 94618 [details] [review]
Bug 23089: Fix sorting on sum and regular fields

As of bug 20589 we no longer analyze sort fields and so we no longer need to append ".phrase"
to our sort in searches.

Additionally, sort fields based on 'sum' should also use sum in building the value to sort on

To test:
0 - Be using ES
1 - Find the most circulated item in your collection
2 - Search for '*'
3 - Sort by popularity DESC
4 - Note that item is not first
5 - Try to sort by anything but relevancy, it fails
6 - Apply patch
7 - Redo searches and sorts
8 - Things should now work as expected

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 7 Ere Maijala 2019-10-24 10:36:29 UTC
Created attachment 94653 [details] [review]
Bug 23089: Fix QueryBuilder tests.
Comment 8 Ere Maijala 2019-10-24 10:37:32 UTC
I noticed that QueryBuilder tests were failing since they expected the __phrase suffix.
Comment 9 Nick Clemens 2019-10-24 11:22:20 UTC
Created attachment 94656 [details] [review]
Bug 23089: Fix QueryBuilder tests.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2019-10-29 09:48:19 UTC
These patches address issue on master, we will still need a patch for popularity sort in 19.05 and earlier
Comment 11 Martin Renvoize 2019-11-01 10:46:38 UTC
Created attachment 94944 [details] [review]
Bug 23089: Unit tests

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-11-01 10:46:42 UTC
Created attachment 94945 [details] [review]
Bug 23089: Fix sorting on sum and regular fields

As of bug 20589 we no longer analyze sort fields and so we no longer need to append ".phrase"
to our sort in searches.

Additionally, sort fields based on 'sum' should also use sum in building the value to sort on

To test:
0 - Be using ES
1 - Find the most circulated item in your collection
2 - Search for '*'
3 - Sort by popularity DESC
4 - Note that item is not first
5 - Try to sort by anything but relevancy, it fails
6 - Apply patch
7 - Redo searches and sorts
8 - Things should now work as expected

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-11-01 10:46:46 UTC
Created attachment 94946 [details] [review]
Bug 23089: Fix QueryBuilder tests.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-11-01 10:47:33 UTC
Works as expected, has tests, passes tests and passes QA script.

Passing QA
Comment 15 Martin Renvoize 2019-11-03 07:47:21 UTC
Nice work!

Pushed to master for 19.11.00
Comment 16 Fridolin Somers 2019-11-08 16:04:17 UTC
Can it be applyed on 19.05.x even without Bug 20589 that added changed admin/searchengine/elasticsearch/field_config.yaml
Comment 17 Nick Clemens 2019-11-13 14:31:57 UTC
Created attachment 95390 [details] [review]
Bug 23089: [19.05 and earlier] Use integer field directly for sorting
Comment 18 Nick Clemens 2019-11-13 14:32:43 UTC
(In reply to Fridolin SOMERS from comment #16)
> Can it be applyed on 19.05.x even without Bug 20589 that added changed
> admin/searchengine/elasticsearch/field_config.yaml

No, it relies on other logic there, but I made a new patch to bridge the behavior. I tried to keep it as small as possible.
Comment 19 Jonathan Druart 2019-12-18 13:41:22 UTC
Nick, tests are failing on 18.11.x

t/Koha/SearchEngine/Elasticsearch.t .. 3/4     # Looks like you planned 51 tests but ran 10.

#   Failed test 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests'
#   at t/Koha/SearchEngine/Elasticsearch.t line 518.
Not a HASH reference at t/Koha/SearchEngine/Elasticsearch.t line 335.
# Looks like your test exited with 255 just after 4.
Comment 20 Jonathan Druart 2019-12-18 13:43:25 UTC
This patchset has been pushed to 19.05.05 and 18.11.11
Comment 21 Lucas Gass 2019-12-18 14:39:45 UTC
This is not in 19.05.05.

It has been backported to 19.05.x for 19.05.06
Comment 22 Nick Clemens 2019-12-18 15:02:13 UTC
Created attachment 96423 [details] [review]
Bug 23089: [18.11.x] fix tests
Comment 23 Hayley Pelham 2019-12-18 21:39:22 UTC
This patchset has not been pushed to 18.11.11, rather it has been pushed to 18.11.x for 18.11.12
Comment 24 Nick Clemens 2020-01-16 11:43:37 UTC
Note that sites receiving this patch will need to rebuild their ES indexes due to the change to field_config