Bug 23089 - Elasticsearch - cannot sort on non-text fields
Summary: Elasticsearch - cannot sort on non-text fields
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords: rel_19_05_candidate
Depends on:
Blocks: 23875 23887
  Show dependency treegraph
 
Reported: 2019-06-10 12:30 UTC by Nick Clemens
Modified: 2019-11-13 14:32 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 23089: Unit test to highlight sorting issue (1.73 KB, patch)
2019-06-10 12:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23089: Unit tests (3.09 KB, patch)
2019-10-10 17:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23089: Fix sorting on sum and regular fields (2.57 KB, patch)
2019-10-10 17:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23089: Unit tests (3.14 KB, patch)
2019-10-23 13:41 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 23089: Fix sorting on sum and regular fields (2.62 KB, patch)
2019-10-23 13:41 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 23089: Fix QueryBuilder tests. (1.10 KB, patch)
2019-10-24 10:36 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 23089: Fix QueryBuilder tests. (1.16 KB, patch)
2019-10-24 11:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23089: Unit tests (3.37 KB, patch)
2019-11-01 10:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23089: Fix sorting on sum and regular fields (2.69 KB, patch)
2019-11-01 10:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23089: Fix QueryBuilder tests. (1.22 KB, patch)
2019-11-01 10:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23089: [19.05 and earlier] Use integer field directly for sorting (2.40 KB, patch)
2019-11-13 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.