Bug 20589 - Add field boosting and use elastic query fields parameter instead of deprecated _all
Summary: Add field boosting and use elastic query fields parameter instead of deprecat...
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 18316
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-16 15:11 UTC by David Gustafsson
Modified: 2018-12-10 15:54 UTC (History)
6 users (show)

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


Attachments
Bug 20589: Add field boosting and use query_string fields parameter (27.31 KB, patch)
2018-04-16 15:39 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (27.31 KB, patch)
2018-04-16 16:02 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (27.53 KB, patch)
2018-04-17 07:38 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (31.18 KB, patch)
2018-04-17 18:07 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (31.15 KB, patch)
2018-04-18 09:19 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (31.71 KB, patch)
2018-04-18 09:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (40.60 KB, patch)
2018-11-19 14:19 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (40.14 KB, patch)
2018-11-20 14:03 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (41.04 KB, patch)
2018-12-10 14:09 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20589: Add field boosting and use query_string fields parameter (41.04 KB, patch)
2018-12-10 15:53 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2018-04-16 15:11:11 UTC
Generate a list of fields for the query_string query fields parameter, with possible boosts, instead of using "_all"-field. Also add "search" flag in search_marc_to_field table so that certain mappings can be excluded from searches. Add option to include/exclude fields in query_string "fields" parameter depending on searching in OPAC or staff client. Refactor code to remove all other dependencies on "_all"-field.

(the atomicupdate sql-file is a bit obfuscated since had to be able to run in multiple time without error because of our local deploy-script, when moving this all procedure related code can be removed, only the "ALTER TABLE.."-lines are relevant.)
Comment 1 David Gustafsson 2018-04-16 15:39:03 UTC
Created attachment 74245 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) No reindex is really required, but still, reindex authorities and
   biblios to verify that this still works.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 2 David Gustafsson 2018-04-16 16:02:38 UTC
Created attachment 74249 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) No reindex is really required, but still, reindex authorities and
   biblios to verify that this still works.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 3 David Gustafsson 2018-04-17 07:38:38 UTC
Created attachment 74301 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) No reindex is really required, but still, reindex authorities and
   biblios to verify that this still works.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 4 David Gustafsson 2018-04-17 07:40:29 UTC
Added a *.perl update script instead of the custom SQL-script.
Comment 5 David Gustafsson 2018-04-17 18:07:02 UTC
Created attachment 74373 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) No reindex is really required, but still, reindex authorities and
   biblios to verify that this still works.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 6 David Gustafsson 2018-04-17 18:11:46 UTC
Had forgotten to take new "search" field into account for mappings, and also fixed where had not removed all of the _all-field mapping (which would have caused indexing to crash).
Comment 7 David Gustafsson 2018-04-18 09:19:00 UTC
Created attachment 74418 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) No reindex is really required, but still, reindex authorities and
   biblios to verify that this still works.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 8 David Gustafsson 2018-04-18 09:23:24 UTC
Discovered search where missing in '+as', and '+select' clause, which causes indexing to crash, will fix.
Comment 9 David Gustafsson 2018-04-18 09:24:01 UTC
Created attachment 74419 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) No reindex is really required, but still, reindex authorities and
   biblios to verify that this still works.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 10 Alex Arnaud 2018-04-19 14:29:26 UTC
Searchable parameter on search fields (staff/opac) is nice.
But, is searchable parameter on mapping useful? We can just remove the mapping, right?

This patch could be rebased on bug 18316, that brings boost for simple search.
Comment 11 David Gustafsson 2018-04-25 19:31:48 UTC
Yes, I think the best option would be to try getting bug_18316 merged, and then refactor this upon that. I completely missed bug_18316 before starting to work on this. I tried applying bug_18316 but was unable to do so because of some git-error, think there might be something wonky with the current patches.
Comment 12 David Gustafsson 2018-04-25 19:38:38 UTC
And about the searchable parameter, I think we are going to need this in the future so included it as did not see a major downside of doing so. The problem this might solve is that we would want to have exclusive "sort"-field mappings, not included in the main field. The idea is to allow mappings to be written on the form "035a|035b|035c", and this would mean that the first field that has a value gets mapped, the rest skipped. We have disabled sorting since it's very unpredictable for multivalued fields, and this was seen as a possible future solution.
Comment 13 Katrin Fischer 2018-04-27 06:23:09 UTC
Hi David, if you write bug 18316 instead of bug_18316, bugzilla will automagically link it. :)
Comment 14 Séverine Queune 2018-04-27 12:48:29 UTC
I a bit lost because of last comments and allusion about merging it with bug 18316 : should this one be tested ?
Comment 15 David Gustafsson 2018-10-29 14:57:41 UTC
I think this patch in it's current state needs to be refactored with regard to the changes introduces in Bug 18316 once has been pushed to master. I will do this, but will probably wait until it has been pushed (which it seems close to be).
Comment 16 David Gustafsson 2018-11-09 16:28:55 UTC
I have started working on this now since 18316 has been merged. Will update beginning of next week.
Comment 17 David Gustafsson 2018-11-19 14:19:08 UTC
Created attachment 82503 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) Reindex authorities and biblios.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 18 David Gustafsson 2018-11-19 14:26:45 UTC
This is still work in progress. Have not adjusted tests for the refactored code for example. There are parts of this patch regarding fixes for authorities search that could be a separate patch, but since removing '_all' also requires adjusting this patch such a patch would probably cause merge conflict with this one. So it could be simple to bundle theses changes as one patch.
Comment 19 David Gustafsson 2018-11-19 14:27:48 UTC
*but since removing '_all' also requires adjusting this _part_ such a patch would probably cause merge conflict with this one. So it could be simple to bundle theses changes as one patch.
Comment 20 David Gustafsson 2018-11-19 14:40:37 UTC
I will write a summary of the changes with motivations behind each one as this can be difficult to understand by just looking at the code. I'm also a little bit confused my self with regards to the correct behaviour of build_auth_query_compat for example. And would probably have to setup a Zebra instance to find out exactly how the different options are supposed to work.
Comment 21 David Gustafsson 2018-11-20 14:03:05 UTC
Created attachment 82544 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) Reindex authorities and biblios.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 22 David Gustafsson 2018-11-20 15:54:53 UTC
Here is a more verbose explaction of most of the changes:

- Remove syspref QueryWeightFields as this seems not to be used anyway.

- Replace hard coded 'biblios' and 'authorities' in mappings.pl with constants.

- Add _search_fields method in QueryParser for returning weighted/unweighted search fields for OPAC/STaffClient (as optional subfield, used primarily for authorities search).

Refactor authorities search. This part is a a litte bit iffy:

What mainly confuses me is the motivation behind the differentiation between is/=, and "exact". With regards to elastic that operator only seems to be used in C4/Matcher.pm. It also occurs in Koha/QueryParser/Driver/PQF.pm and Koha/REST/Plugin/Query.pm but i don't believe Elastic is used as backend for those. What I can gather there where actually no difference between is/= and "exact" before my changes as is/= performed a "term" query on "<field>.phrase" and "exact" performed a "match_phrase" on the same field, which is pointless since the "phrase" subfield is not tokenized (used keyword analyser). So I simplified this and use the same query for both is/= and "exact".

As "exact" queries seems to be used for matching, I also thought it would be more fitting to perform a case insensitive match instead of also removing punctuation (which could result in unexpected matches).

I also noticed that the Authorities type drop-down in the authorities search was not respected, so this is not included in the query if selected.

Also noted there is an $and_or option to build_authorities_query_compat that is not used, this should probably be addressed, but not fixed in this patch.

Tidy up the index/fields configuration a little bit:

- Rename all occurrences of misspelled "analyser" to "analyzer".
- Rename "my_normalizer" to more descriptive "nfkc_cf_normalizer".
- Rename "normalizer_keyword" to more descriptive "icu_folding_normalizer".
- Rename subfield "lc_raw" to "ci_raw" (case insensitive). I don't think this subfield actually was case insensitive before since "my_normalizer" was used wich is only performing UTF normalization, not case folding.
- Don't use "phrase" subfield for sorting, this is incorrect and will result in strange results since byte order of characters is used. There is also no need for a subfield for the sort field. I removed the subfield and changed type to "icu_collation_keyword", which will attempt to sort in order with the least conflicts between languages. Ideally there should be a syspref for setting collation language to sort by. This is a pretty trivial fix so might open a new issue for that.

As a result of this "phrase" is almost not used at all, and could probably be removed with phrase queries on the search field instead in a later stage (which would decrease index size considerably).
Comment 23 David Gustafsson 2018-12-10 14:09:09 UTC
Created attachment 83016 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) Reindex authorities and biblios.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 24 David Gustafsson 2018-12-10 14:09:44 UTC
Rebase against master (but currently untested).
Comment 25 David Gustafsson 2018-12-10 15:53:36 UTC
Created attachment 83019 [details] [review]
Bug 20589: Add field boosting and use query_string fields parameter

Generate a list of fields for the query_string query fields parameter,
with possible boosts, instead of using "_all"-field. Also add "search"
flag in search_marc_to_field table so that certain mappings can be
excluded from searches. Add option to include/exclude fields in
query_string "fields" parameter depending on searching in OPAC or staff
client. Refactor code to remove all other dependencies on "_all"-field.

How to test:
1) Reindex authorities and biblios.
2) Search biblios and try to verify that this works as expected.
3) Search authorities and try to verify that this works as expected.
4) Go to "Search engine configuration"
5) Change some "Boost", "Staff client", and "OPAC" settings and save.
6) Verify that those settings where saved accordingly.
7) Click the "Biblios" or "Authorities" tab and change one or more
   "Searchable" settings
8) Verfiy that those settings where saved accordingly.
9) Try to verify that these settings has taken effect by peforming
   some biblios and/or authorities searches.

Sponsorded-by: Gothenburg Univesity Library
Comment 26 David Gustafsson 2018-12-10 15:54:23 UTC
Fixed incorrectly named is_mapped_biblios to mapped_biblios in mappings.tt.