Bug 19604 - Elasticsearch Fixes for build_authorities_query for auth searching
Summary: Elasticsearch Fixes for build_authorities_query for auth searching
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Nick Clemens (kidclamp)
QA Contact:
URL:
Keywords:
: 21103 (view as bug list)
Depends on: 19582 20073
Blocks: 20273 21103
  Show dependency treegraph
 
Reported: 2017-11-09 16:23 UTC by Nick Clemens (kidclamp)
Modified: 2019-10-14 19:56 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching (5.75 KB, patch)
2017-11-09 16:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching (5.94 KB, patch)
2017-11-17 14:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 19604: Unit tests (5.21 KB, patch)
2017-11-17 14:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching (6.00 KB, patch)
2018-03-13 16:09 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19604: Unit tests (5.27 KB, patch)
2018-03-13 16:09 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching (6.14 KB, patch)
2018-07-16 11:33 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 19604: Unit tests (5.27 KB, patch)
2018-07-16 11:33 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching (6.19 KB, patch)
2018-07-18 18:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19604: Unit tests (5.33 KB, patch)
2018-07-18 18:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19604: Fix perlcritic "Loop iterator is not lexical" (1.20 KB, patch)
2018-07-31 11:22 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) 2017-11-09 16:23:43 UTC
Some of the searches here were based on older syntax and caused errors
Comment 1 Nick Clemens (kidclamp) 2017-11-09 16:31:43 UTC
Created attachment 69072 [details] [review]
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching

To test:
1 - Do some authority searches in Zebra
2 - Switch to ES and repeat, results will vary and some may fail
3 - Apply patch and dependencies
4 - Reindex ES
5 - Repeat searches, they should suceed and results should be similar to
Zebra
6 - Slight differences are okay, but results should (mostly) meet
expectations

A few notes:
We add a 'normalizer' to ensure we get a single token from the heading
indexes, this makes 'starts with' work as expcted
We switch to 'AND' for fields searched from cataloging editor - this
matches Zebra results
We force the '__sort' fields for sorting - if sorting looks wrong try
reducing the heading field to a single subfield - this will need to be
addressed on a future bug (multiple subfields create an array, ES sorts
        those randomly)
Comment 2 David Bourgault 2017-11-10 21:26:58 UTC
I did a search for 'me' from the OPAC's Auth search.

With Zebra I got 1411 results (some, but not all, matching on 'mé')
With ES I got a single result, in which the word 'Me' was found as-is
Comment 3 Nick Clemens (kidclamp) 2017-11-17 14:10:48 UTC
Created attachment 69198 [details] [review]
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching

To test:
1 - Do some authority searches in Zebra
2 - Switch to ES and repeat, results will vary and some may fail
3 - Apply patch and dependencies
4 - Reindex ES
5 - Repeat searches, they should suceed and results should be similar to
Zebra
6 - Slight differences are okay, but results should (mostly) meet
expectations

A few notes:
We add a 'normalizer' to ensure we get a single token from the heading
indexes, this makes 'starts with' work as expcted
We switch to 'AND' for fields searched from cataloging editor - this
matches Zebra results
We force the '__sort' fields for sorting - if sorting looks wrong try
reducing the heading field to a single subfield - this will need to be
addressed on a future bug (multiple subfields create an array, ES sorts
        those randomly)
Comment 4 Nick Clemens (kidclamp) 2017-11-17 14:10:51 UTC
Created attachment 69199 [details] [review]
Bug 19604: Unit tests

1 - Apply previous patches
2 - prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
3 - Should be green
4 - high fives!
Comment 5 Nick Clemens (kidclamp) 2017-11-17 14:13:45 UTC
(In reply to David Bourgault from comment #2)
> I did a search for 'me' from the OPAC's Auth search.
> 
> With Zebra I got 1411 results (some, but not all, matching on 'mé')
> With ES I got a single result, in which the word 'Me' was found as-is

I altered the 'contains' search to be a wildcard and break terms - it should be closer now but won't mirror exactly zebra
Comment 6 David Bourgault 2017-11-17 20:14:55 UTC
Tests passed, QA tools green.

However, whereas I had 1 exact 1, I now have 0 search results.

I tried reseting the mappings, I reindexed -d -a -v twice but I can't get it to find results.

Testing in Kibana with the query `Heading:*me*`, I got multiple results (including both 'me' and 'mé') so I don't think it's a problem specific to my index/database.

Failed QA :(
Comment 7 Nick Clemens (kidclamp) 2017-11-21 15:34:40 UTC
(In reply to David Bourgault from comment #6)
> Tests passed, QA tools green.
> 
> However, whereas I had 1 exact 1, I now have 0 search results.
> 
> I tried reseting the mappings, I reindexed -d -a -v twice but I can't get it
> to find results.
> 
> Testing in Kibana with the query `Heading:*me*`, I got multiple results
> (including both 'me' and 'mé') so I don't think it's a problem specific to
> my index/database.
> 
> Failed QA :(

Can you let me know the exact search and terms you are using? and the data in your DB? with patches applied and default data from kohadevbox I get results for 'me' or 'mé' and all are as expected
Comment 8 Nick Clemens (kidclamp) 2018-01-19 18:06:09 UTC
Still applies - queries return well for me, moving back to needs signoff
Comment 9 Nicolas Legrand 2018-03-13 16:09:45 UTC
Created attachment 72807 [details] [review]
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching

To test:
1 - Do some authority searches in Zebra
2 - Switch to ES and repeat, results will vary and some may fail
3 - Apply patch and dependencies
4 - Reindex ES
5 - Repeat searches, they should suceed and results should be similar to
Zebra
6 - Slight differences are okay, but results should (mostly) meet
expectations

A few notes:
We add a 'normalizer' to ensure we get a single token from the heading
indexes, this makes 'starts with' work as expcted
We switch to 'AND' for fields searched from cataloging editor - this
matches Zebra results
We force the '__sort' fields for sorting - if sorting looks wrong try
reducing the heading field to a single subfield - this will need to be
addressed on a future bug (multiple subfields create an array, ES sorts
        those randomly)

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 10 Nicolas Legrand 2018-03-13 16:09:50 UTC
Created attachment 72808 [details] [review]
Bug 19604: Unit tests

1 - Apply previous patches
2 - prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
3 - Should be green
4 - high fives!

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 11 Nicolas Legrand 2018-03-13 16:17:28 UTC
Test plans worked as it should with unimarc.

Biggest difference I noticed, שלו (shalev) matches only שלו with unpatched ES and שלו but also שלום (shalom) with Zebra and with ES patched. And also sorting. Latin only authorities tend to be first with ES.
Comment 12 Katrin Fischer 2018-04-12 09:31:26 UTC
Ended here trying to fix the Internal server error I have with every authority search (OPAC, Authorities module and linker plugin). Error is:

[Request] ** [http://localhost:9200]-[400] [query_shard_exception] No mapping found for [Heading__sort.phrase] in order to sort on, with: {"index":"koha_kohadev_authorities","index_uuid":"FwdgBXbyRq-5E0F3sa4LTA"}, called from sub Search::Elasticsearch::Role::Client::Direct::__ANON__ at /usr/share/perl5/Catmandu/Store/ElasticSearch/Bag.pm line 167. With vars: {'status_code' => 400,'body' => {'error' => {'root_cause' => [{'type' => 'query_shard_exception','index' => 'koha_kohadev_authorities','index_uuid' => 'FwdgBXbyRq-5E0F3sa4LTA','reason' => 'No mapping found for [Heading__sort.phrase] in order to sort on'}],'grouped' => bless( do{\(my $o = 1)}, 'JSON::PP::Boolean' ),'reason' => 'all shards failed','phase' => 'query','failed_shards' => [{'shard' => 0,'node' => 'vSTPqetISteesIkQSA_Ehg','reason' => {'index' => 'koha_kohadev_authorities','reason' => 'No mapping found for [Heading__sort.phrase] in order to sort on','index_uuid' => 'FwdgBXbyRq-5E0F3sa4LTA','type' => 'query_shard_exception'},'index' => 'koha_kohadev_authorities'}],'type' => 'search_phase_execution_exception'},'status' => 400},'request' => {'ignore' => [],'method' => 'GET','mime_type' => 'application/json','qs' => {},'path' => '/koha_kohadev_authorities/data/_search','serialize' => 'std','body' => {'size' => 20,'sort' => [{'Heading__sort.phrase' => 'asc'}],'query' => {'bool' => {'must' => [{'wildcard' => {'Heading-Main.phrase' => '*der*'}}]}},'from' => 0}}}
Unable to perform your search. Please try again.

Can you help?
Comment 13 Katrin Fischer 2018-04-12 09:33:49 UTC
Tried a full reindex, bibliographic search works.
Comment 14 Ere Maijala 2018-04-12 11:15:01 UTC
Katrin, I have some related improvements in bug 19365, but I'm waiting for this to get pushed first before rebasing them. If this and that's not enough, I should have something more to contribute since we have them working quite nicely locally.
Comment 15 Bouzid Fergani 2018-04-13 19:52:00 UTC
Hi,

I not obtain the same result as zebra with applying this patch.
I try the simple search : test using operator contain
 in zebra i have : 4 results
 In ES, I have : 2 results
without this patch, I obtain :
  In zebra i have : 4 results
  In ES, I have : 1 result
Thanks.
Bouzid.
Comment 16 Katrin Fischer 2018-04-16 05:48:54 UTC
(In reply to Bouzid Fergani from comment #15)
> Hi,
> 
> I not obtain the same result as zebra with applying this patch.
> I try the simple search : test using operator contain
>  in zebra i have : 4 results
>  In ES, I have : 2 results
> without this patch, I obtain :
>   In zebra i have : 4 results
>   In ES, I have : 1 result
> Thanks.
> Bouzid.

Hi Bouzid, it's hard to tell, if that's right or wrong from your description.. We can't always expect the same results. What was your search? On which kind of data? Which results where missing you had expected to be included?
Comment 17 Alex Arnaud 2018-05-03 12:41:32 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2018-07-15 15:46:24 UTC
Basic searches for authorities with a sort not being 'none' fail and block other bugs. Patches here currently don't apply, can someone take a look?
Comment 19 Nick Clemens (kidclamp) 2018-07-16 11:33:53 UTC
Created attachment 76977 [details] [review]
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching

To test:
1 - Do some authority searches in Zebra
2 - Switch to ES and repeat, results will vary and some may fail
3 - Apply patch and dependencies
4 - Reindex ES
5 - Repeat searches, they should suceed and results should be similar to
Zebra
6 - Slight differences are okay, but results should (mostly) meet
expectations

A few notes:
We add a 'normalizer' to ensure we get a single token from the heading
indexes, this makes 'starts with' work as expcted
We switch to 'AND' for fields searched from cataloging editor - this
matches Zebra results
We force the '__sort' fields for sorting - if sorting looks wrong try
reducing the heading field to a single subfield - this will need to be
addressed on a future bug (multiple subfields create an array, ES sorts
        those randomly)

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 20 Nick Clemens (kidclamp) 2018-07-16 11:33:57 UTC
Created attachment 76978 [details] [review]
Bug 19604: Unit tests

1 - Apply previous patches
2 - prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
3 - Should be green
4 - high fives!

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 21 Katrin Fischer 2018-07-16 20:46:38 UTC
Tests pass, but I can't get authorities to sort. I am sorry, can you please check if it works for you?

I tried ... resetting indexes, reindexing multiple times, multiple reset_all, really stuck here :(
Comment 22 Katrin Fischer 2018-07-18 17:52:06 UTC
Giving this another go... keep fingers crossed!
Comment 23 Katrin Fischer 2018-07-18 18:09:32 UTC
Nick and I found the key:

DELETE from search_marc_to_field; DELETE from search_marc_map; delete from search_field;
localhost:8081/cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1
perl misc/search_tools/rebuildelasticsearch.pl -v -d

I can search authorities now, sorting them as I see fit :)
Comment 24 Katrin Fischer 2018-07-18 18:18:04 UTC
Created attachment 77088 [details] [review]
Bug 19604: Elasticsearch Fixes for build_authorities_query for auth searching

To test:
1 - Do some authority searches in Zebra
2 - Switch to ES and repeat, results will vary and some may fail
3 - Apply patch and dependencies
4 - Reindex ES
5 - Repeat searches, they should suceed and results should be similar to
Zebra
6 - Slight differences are okay, but results should (mostly) meet
expectations

A few notes:
We add a 'normalizer' to ensure we get a single token from the heading
indexes, this makes 'starts with' work as expcted
We switch to 'AND' for fields searched from cataloging editor - this
matches Zebra results
We force the '__sort' fields for sorting - if sorting looks wrong try
reducing the heading field to a single subfield - this will need to be
addressed on a future bug (multiple subfields create an array, ES sorts
        those randomly)

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2018-07-18 18:18:11 UTC
Created attachment 77089 [details] [review]
Bug 19604: Unit tests

1 - Apply previous patches
2 - prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
3 - Should be green
4 - high fives!

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Nick Clemens (kidclamp) 2018-07-19 17:36:33 UTC
Awesome work all!

Pushed to master for 18.11
Comment 27 Jonathan Druart 2018-07-31 11:22:01 UTC
Created attachment 77328 [details] [review]
Bug 19604: Fix perlcritic "Loop iterator is not lexical"

Loop iterator is not lexical at line 318, column 13.  See page 108 of
PBP.  (Severity: 5)
Comment 28 Tomás Cohen Arazi (tcohen) 2018-07-31 13:46:29 UTC
Follow-up pushed to master. Gracias, Jonathan!
Comment 29 Martin Renvoize (ashimema) 2018-08-02 14:12:00 UTC
This looks a lot like a bug rather than an enhancement, however with it depending on bug 20073 which certainly is an enhancement I am struggling to apply it to the 18.05.x branch.

Can anyone more familiar with ES confirm whether 18.05.x is affected?
Comment 30 Nicolas Legrand 2018-08-06 09:28:08 UTC
Ay up Martin,

When I did test it against 17.12 it was very relevant and I quickly rebased it on 18.05.00 to have it in our production 18.05 before it will be rebased on Bug 20073.

Nick should confirm this, but it seems to me his last patch is only a rebase against Bug 20073, so maybe you could use the previous patch, or what I have done : https://github.com/BULAC/koha/commits/18.05.00_bug_19604

But maybe I am suggesting something terrible in the whole development process?
Comment 31 Nick Clemens (kidclamp) 2018-08-06 11:21:11 UTC
(In reply to Nicolas Legrand from comment #30)
> Ay up Martin,
> 
> When I did test it against 17.12 it was very relevant and I quickly rebased
> it on 18.05.00 to have it in our production 18.05 before it will be rebased
> on Bug 20073.
> 
> Nick should confirm this, but it seems to me his last patch is only a rebase
> against Bug 20073, so maybe you could use the previous patch, or what I have
> done : https://github.com/BULAC/koha/commits/18.05.00_bug_19604
> 
> But maybe I am suggesting something terrible in the whole development
> process?

This is what I was going to suggest as well - it is mostly moving the changes from the module into the files, earlier patches should work, just check the tests to ensure nothing else changed and let me knwo if you need me to take a crack
Comment 32 Martin Renvoize (ashimema) 2018-08-06 11:25:39 UTC
Excellent, thanks Nicolas and Nick. I thought it was probably as simple as that but as I'm not fully up to speed on ES myself yet it was good to have it verified. The repo link is very helpful, I'll get this pushed today :)
Comment 33 Martin Renvoize (ashimema) 2018-08-06 12:55:59 UTC
Pushed to 18.05.x for 18.05.03. Thanks guys.
Comment 34 Mark Tompsett 2018-09-06 01:22:29 UTC
*** Bug 21103 has been marked as a duplicate of this bug. ***
Comment 35 Fridolin Somers 2018-09-21 10:05:35 UTC
Enhancement not pushed to 17.11.x