issues is defined as an sum field, this is interpreted to ES as integer, however, the issues__sort field is stored as keyword. Attempting to order search results by 'popularity' fails from ES - because we can't use text fields, we have to use a 'keyword' field. In our biblio search indexes this is fine because our __sort fields all get a phrase keyword field We onyl set field__sort.phrase for text fields though, so issues__sort doesn't use the phrase field, even though it exists Adding a test to try to ilustrate what I am saying here To recreate: 1 - Enable and index ES 2 - Search on staff client or OPAC 3 - Attempt to sort by popularity 4 - It fails, check the error logs to see why
Created attachment 90451 [details] [review] Bug 23089: Unit test to highlight sorting issue
Created attachment 93982 [details] [review] Bug 23089: Unit tests
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
Bumping relevancy, we must fix sorting before release
Created attachment 94617 [details] [review] Bug 23089: Unit tests Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
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>
Created attachment 94653 [details] [review] Bug 23089: Fix QueryBuilder tests.
I noticed that QueryBuilder tests were failing since they expected the __phrase suffix.
Created attachment 94656 [details] [review] Bug 23089: Fix QueryBuilder tests. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
These patches address issue on master, we will still need a patch for popularity sort in 19.05 and earlier
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>
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>
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>
Works as expected, has tests, passes tests and passes QA script. Passing QA
Nice work! Pushed to master for 19.11.00
Can it be applyed on 19.05.x even without Bug 20589 that added changed admin/searchengine/elasticsearch/field_config.yaml
Created attachment 95390 [details] [review] Bug 23089: [19.05 and earlier] Use integer field directly for sorting
(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.
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.
This patchset has been pushed to 19.05.05 and 18.11.11
This is not in 19.05.05. It has been backported to 19.05.x for 19.05.06
Created attachment 96423 [details] [review] Bug 23089: [18.11.x] fix tests
This patchset has not been pushed to 18.11.11, rather it has been pushed to 18.11.x for 18.11.12
Note that sites receiving this patch will need to rebuild their ES indexes due to the change to field_config