Bug 18189 - Elasticsearch sorting broken
Summary: Elasticsearch sorting broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17255
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-01 14:33 UTC by Nick Clemens
Modified: 2018-06-04 20:10 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18189 - Elasticsearch sorting broken (1.37 KB, patch)
2017-03-01 14:37 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18189 - Elasticsearch sorting broken (1.43 KB, patch)
2017-03-23 02:01 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 18189 - Elasticsearch sorting broken (1.49 KB, patch)
2017-03-29 16:30 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 2017-03-01 14:33:03 UTC
Current setting in elasticsearch mapping use 'text' type for sort fields, this seems like extra overhead.

If the fields are only used for sorting we can index them as 'keyword' (exact phrase) and this has the side effect of fixing sorting
Comment 1 Nick Clemens 2017-03-01 14:37:23 UTC
Created attachment 60780 [details] [review]
Bug 18189 - Elasticsearch sorting broken

To test:
1 - Have ES enabled and biblios and authorities indexed
2 - Perform a catalog search
3 - Try to sort results by author, it will fail
4 - Perform an authority search with sorting enabled, it will fail
5 - Apply patch
6 - Reindex, deleting current indexes
    perl /home/vagrant/kohaclone/misc/search_tools/rebuild_elastic_search.pl
    -d -v
7 - Repeat 3&4, they should succeed
Comment 2 Nick Clemens 2017-03-01 14:42:27 UTC
Note this won't fix all sorting options unless they are defined as sortable in the config.

acqdate fails out of the box - set acqdate as sortable - reindex - it should now work

I suppose we either don't allow altering sortability for some fields, or the dropdown must populate based on what is defined as sortable.
Comment 3 Srdjan Jankovic 2017-03-21 00:56:19 UTC
I'm getting:
Fielddata is disabled on text fields by default. Set fielddata=true on [author.phrase] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory.

Is something wrong with my es config?
Comment 4 Nick Clemens 2017-03-21 13:15:01 UTC
(In reply to Srdjan Jankovic from comment #3)
> I'm getting:
> Fielddata is disabled on text fields by default. Set fielddata=true on
> [author.phrase] in order to load fielddata in memory by uninverting the
> inverted index. Note that this can however use significant memory.
> 
> Is something wrong with my es config?

Is this before or after the patch? I cannot seem to replicate the issue.
Comment 5 Srdjan Jankovic 2017-03-22 01:02:13 UTC
(In reply to Nick Clemens from comment #4)
> (In reply to Srdjan Jankovic from comment #3)
> > I'm getting:
> > Fielddata is disabled on text fields by default. Set fielddata=true on
> > [author.phrase] in order to load fielddata in memory by uninverting the
> > inverted index. Note that this can however use significant memory.
> > 
> > Is something wrong with my es config?
> 
> Is this before or after the patch? I cannot seem to replicate the issue.

Both. What was the error that you were trying to get rid of?
Comment 6 Nick Clemens 2017-03-22 09:00:50 UTC
Before this patch passing sort options to Elastic gives an error much like the one you mentioned. If you are getting this before and after the patch I would suspect it is an issue with modules/versions, and or outdated mappings.

Using 
admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1

should get your mappings up to date, then delete and rebuild indexes, and you should be good

I would also check versions against kohadevbox elastic setup:
https://github.com/digibib/kohadevbox/blob/master/roles/kohadevbox/tasks/elasticsearch-2.x.yml
Comment 7 Srdjan Jankovic 2017-03-23 00:54:51 UTC
Hm, done the reset thing, and now getting:
No mapping found for [author__sort.phrase] in order to sort on, with: {"index":"koha_biblios","index_uuid":"fehRdZHCRlGe1FxHYiKbMA"}
Comment 8 Srdjan Jankovic 2017-03-23 02:01:10 UTC
Created attachment 61494 [details] [review]
[SIGNED-OFF] Bug 18189 - Elasticsearch sorting broken

To test:
1 - Have ES enabled and biblios and authorities indexed
2 - Perform a catalog search
3 - Try to sort results by author, it will fail
4 - Perform an authority search with sorting enabled, it will fail
5 - Apply patch
6 - Reindex, deleting current indexes
    perl /home/vagrant/kohaclone/misc/search_tools/rebuild_elastic_search.pl
    -d -v
7 - Repeat 3&4, they should succeed

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Comment 9 Srdjan Jankovic 2017-03-23 02:02:17 UTC
Sorry, my fault, omitted -d when reindexing.
Comment 10 Jonathan Druart 2017-03-29 16:20:25 UTC
What about integers and dates?
Comment 11 Jonathan Druart 2017-03-29 16:29:47 UTC
Does only work with author, our other fields are not marked as sortable by default (?!?)

Moreover "Ó Cadhain, Máirtín" is sorted after "Yerby, Frank Garvin", terrible...
Comment 12 Jonathan Druart 2017-03-29 16:30:36 UTC
Created attachment 61689 [details] [review]
Bug 18189 - Elasticsearch sorting broken

To test:
1 - Have ES enabled and biblios and authorities indexed
2 - Perform a catalog search
3 - Try to sort results by author, it will fail
4 - Perform an authority search with sorting enabled, it will fail
5 - Apply patch
6 - Reindex, deleting current indexes
    perl /home/vagrant/kohaclone/misc/search_tools/rebuild_elastic_search.pl
    -d -v
7 - Repeat 3&4, they should succeed

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Kyle M Hall 2017-03-31 15:13:56 UTC
Pushed to master for 17.05, thanks Nick!
Comment 14 Katrin Fischer 2017-04-02 17:49:13 UTC
Is enh correct here or should this not be a bug?
Comment 15 Jonathan Druart 2017-04-03 12:50:17 UTC
Sort is broken, it's a bug.
Comment 16 Katrin Fischer 2017-04-10 21:08:16 UTC
Ok, missed the dependency on bug 17255 earlier - should we still push this to stable?
Comment 17 Katrin Fischer 2017-07-16 12:07:26 UTC
3 months - still no answer here. Closing.