Bug 20589

Summary: Add field boosting and use elastic query fields parameter instead of deprecated _all
Product: Koha Reporter: David Gustafsson <glasklas>
Component: Searching - ElasticsearchAssignee: David Gustafsson <glasklas>
Status: Pushed to master --- QA Contact: Alex Arnaud <alex.arnaud>
Severity: enhancement    
Priority: P5 - low CC: alex.arnaud, bjorn.nylen, black23, e.betemps, ere.maijala, katrin.fischer, marjorie.barry-vila, martin.renvoize, nick, nicolas.legrand, severine.queune
Version: masterKeywords: Manual, release-notes-needed
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18316
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19604
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20388
Change sponsored?: --- Patch complexity: ---
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
19.11.00
Bug Depends on: 18316    
Bug Blocks:    
Attachments: Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: fix QueryBuilder tests
Bug 20589: Exclude boolean fields from search fields
Bug 20589: Remove QueryWeightFields syspref since no longer in use
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: fix QueryBuilder tests
Bug 20589: Exclude boolean fields from search fields
Bug 20589: Add option for searching entire record if record stored as array
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: fix QueryBuilder tests
Bug 20589: Exclude boolean fields from search fields
Bug 20589: Add option for searching entire record if record stored as array
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: fix QueryBuilder tests
Bug 20589: Exclude boolean fields from search fields
Bug 20589: Add option for searching entire record if record stored as array
Bug 20589: DO NOT PUSH Schema changes
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: fix QueryBuilder tests
Bug 20589: Exclude boolean fields from search fields
Bug 20589: Add option for searching entire record if record stored as array
Bug 20589: DO NOT PUSH Schema changes
Bug 20589: Remove expanded_facet variable and fix tests
Bug 20589: Add field boosting and use query_string fields parameter
Bug 20589: fix QueryBuilder tests
Bug 20589: Exclude boolean fields from search fields
Bug 20589: Add option for searching entire record if record stored as array
Bug 20589: DO NOT PUSH Schema changes
Bug 20589: Remove expanded_facet variable and fix tests
Bug 20589: (RM follow-up) Fix Koha::SearchField.t
Bug 20589: (follow-up) Fix tests

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.
Comment 27 Nick Clemens 2019-03-27 12:47:49 UTC
David, one request here would be to include the opac/staff columns to the mapping yaml file as well - changes that can be lost on a mapping reload are harder to track and keep in place
Comment 28 David Gustafsson 2019-03-27 13:09:00 UTC
Yes, agree. I had forgotten about that. Should be easy to fix. Might be able to do so Friday or next week.
Comment 29 David Gustafsson 2019-04-02 16:46:08 UTC
Just rebased this, but need to test some more and make sure tests are not broken. Should have that done by tomorrow.
Comment 30 David Gustafsson 2019-04-03 14:29:49 UTC
Created attachment 87358 [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 31 David Gustafsson 2019-04-03 14:31:28 UTC
Not ready for signoff though. One test fails, I know the reason but it's not trivial to fix (association need to be used in test data and don't know if that is possible with TestBuilder).
Comment 32 David Gustafsson 2019-04-04 13:16:49 UTC
Created attachment 87411 [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 33 David Gustafsson 2019-04-04 13:16:53 UTC
Created attachment 87412 [details] [review]
Bug 20589: fix QueryBuilder tests
Comment 34 David Gustafsson 2019-04-04 13:58:49 UTC
Created attachment 87415 [details] [review]
Bug 20589: Exclude boolean fields from search fields
Comment 35 David Gustafsson 2019-04-04 13:58:53 UTC
Created attachment 87416 [details] [review]
Bug 20589: Remove QueryWeightFields syspref since no longer in use
Comment 36 Alex Arnaud 2019-04-04 16:04:25 UTC
Hello David,

First: i like this work :). 
The idea of targeting only searchable fields instead of systematically fall back on the entire record make results more consistent (even with boosting).

however, here are some observations:

When searching on a specific field (i.e "Autrement" as publisher), we keep all others searchable fields in the target list:

    "query_string": {
      "query": "(publisher:autrement)",
      "fields": [
        "LC-card-number",
        "Local-number",
        "publisher",
        + all searchable fields
      ]
    }
That has no matter on the results here since the following query returns the same results:

    "query_string": {
      "query": "(publisher:autrement)",
    }

But keeping field list seems to be useless.

A better thing would be to use only fields list instead of having the target in query. Like that:

    "query_string": {
      "query": "(autrement)",
      "fields": [
          publisher
      ]
    }

This also returns the same result (for me) and allows user to make more complete searches such like "autrement AND jean"
If i request this:

    "query_string": {
      "query": "(publisher:autrement OR jean)"
      "fields": [
        "LC-card-number",
        "Local-number",
        "publisher",
        + all searchable
      ],
    }
I get many irrelevant results (more than 600 in my database that are not related with what i typed)
Or this one:

    "query_string": {
      "query": "(autrement OR jean)"
      "fields": [
        "publisher"
      ],
    }
Returns only records containing "Autrement" or "jean" in publisher.
Seems more correct to me.

Sorry for the long comment, waiting for your feedback
Comment 37 David Gustafsson 2019-04-10 12:03:20 UTC
@Alex Arnaud

Thanks for the comments.
Regarding the query "(publisher:autrement OR jean)" I think it should be written as "publisher:(autrement OR jean)" to get the results you want. Also, surrounding parentheses can be left out of all the queries. They are only needed if you need to group sub-expressions. I don't think that Koha should try to reduce the field-list by parsing the query, this greatly increases complexity without any real  benefit.
Comment 38 Nick Clemens 2019-08-23 16:46:15 UTC
Created attachment 92454 [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 39 Nick Clemens 2019-08-23 16:46:19 UTC
Created attachment 92455 [details] [review]
Bug 20589: fix QueryBuilder tests
Comment 40 Nick Clemens 2019-08-23 16:46:23 UTC
Created attachment 92456 [details] [review]
Bug 20589: Exclude boolean fields from search fields
Comment 41 Nick Clemens 2019-08-23 16:46:27 UTC
Created attachment 92457 [details] [review]
Bug 20589: Add option for searching entire record if record stored as array
Comment 42 Nick Clemens 2019-08-23 16:51:48 UTC
Rebased and tested, this works excellently, I would love to move this forward, but I see a few issues in the patches:
1 - You removed QueryWeightedFields, but Zebra uses this, I obsoleted that patch
2 - There are changes to the authority searches here, things are improved, but a bit too strict, they don't relate to this bug, can you move those to a new report?
3 - Schema changes are mixed into first patch, they should be their own so RM can generate separately
4 - I did a minor rebase on top of 22258 and added a followup to allow searchig the entire record if using marc array

Let me know if this makes sense or if I went too far anywhere
Comment 43 David Gustafsson 2019-09-30 12:45:44 UTC
Created attachment 93238 [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 44 David Gustafsson 2019-09-30 12:45:48 UTC
Created attachment 93239 [details] [review]
Bug 20589: fix QueryBuilder tests
Comment 45 David Gustafsson 2019-09-30 12:45:52 UTC
Created attachment 93240 [details] [review]
Bug 20589: Exclude boolean fields from search fields
Comment 46 David Gustafsson 2019-09-30 12:45:56 UTC
Created attachment 93241 [details] [review]
Bug 20589: Add option for searching entire record if record stored as array
Comment 47 David Gustafsson 2019-09-30 12:57:48 UTC
(In reply to Nick Clemens from comment #42)
> Rebased and tested, this works excellently, I would love to move this
> forward, but I see a few issues in the patches:
> 1 - You removed QueryWeightedFields, but Zebra uses this, I obsoleted that
> patch
> 2 - There are changes to the authority searches here, things are improved,
> but a bit too strict, they don't relate to this bug, can you move those to a
> new report?
> 3 - Schema changes are mixed into first patch, they should be their own so
> RM can generate separately
> 4 - I did a minor rebase on top of 22258 and added a followup to allow
> searchig the entire record if using marc array
> 
> Let me know if this makes sense or if I went too far anywhere

Thanks for the feedback! Rebased aagainst master since did not apply again.

1) Ok, I obviously missed that used by Zebra, thanks!

2) I gave this a lot of thought, but on the other hand not sure got it right. It would be possible to break out some changes introduced into separate issues, but they would all depend on each other and would be harder to maintain. So it's bundled together for practical reasons and time constraints. I might have time later on to split things up, but unfortunately not at the moment.

3) Forgot to fix this, but will do that.

4) Great, thanks :)
Comment 48 David Gustafsson 2019-09-30 13:25:34 UTC
Created attachment 93247 [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 49 David Gustafsson 2019-09-30 13:25:38 UTC
Created attachment 93248 [details] [review]
Bug 20589: fix QueryBuilder tests
Comment 50 David Gustafsson 2019-09-30 13:25:42 UTC
Created attachment 93249 [details] [review]
Bug 20589: Exclude boolean fields from search fields
Comment 51 David Gustafsson 2019-09-30 13:25:46 UTC
Created attachment 93250 [details] [review]
Bug 20589: Add option for searching entire record if record stored as array
Comment 52 David Gustafsson 2019-09-30 13:25:50 UTC
Created attachment 93251 [details] [review]
Bug 20589: DO NOT PUSH Schema changes
Comment 53 Nick Clemens 2019-10-01 09:23:54 UTC
Created attachment 93348 [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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Nick Clemens 2019-10-01 09:24:07 UTC
Created attachment 93349 [details] [review]
Bug 20589: fix QueryBuilder tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens 2019-10-01 09:24:20 UTC
Created attachment 93350 [details] [review]
Bug 20589: Exclude boolean fields from search fields

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2019-10-01 09:24:34 UTC
Created attachment 93351 [details] [review]
Bug 20589: Add option for searching entire record if record stored as array

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens 2019-10-01 09:24:41 UTC
Created attachment 93352 [details] [review]
Bug 20589: DO NOT PUSH Schema changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Nick Clemens 2019-10-01 09:25:20 UTC
Created attachment 93353 [details] [review]
Bug 20589: Remove expanded_facet variable and fix tests

Thi is a rebase from 14419 to remove an unused variable

Also update tests to expect the new expected result

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 59 Nick Clemens 2019-10-01 09:27:37 UTC
Added minor followup

Everything seems to work as expected. The authority searching changes seem to be an improvement:
previously an 'exact' search matched any term, now it must match the entire field. Punctuation could be excluded in the future, but exact match should eb exact so I think good

starts with searches and sorting are improved
Comment 60 Alex Arnaud 2019-10-01 15:03:34 UTC
Created attachment 93379 [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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 61 Alex Arnaud 2019-10-01 15:03:43 UTC
Created attachment 93380 [details] [review]
Bug 20589: fix QueryBuilder tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 62 Alex Arnaud 2019-10-01 15:03:52 UTC
Created attachment 93381 [details] [review]
Bug 20589: Exclude boolean fields from search fields

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 63 Alex Arnaud 2019-10-01 15:03:59 UTC
Created attachment 93382 [details] [review]
Bug 20589: Add option for searching entire record if record stored as array

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 64 Alex Arnaud 2019-10-01 15:04:07 UTC
Created attachment 93383 [details] [review]
Bug 20589: DO NOT PUSH Schema changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 65 Alex Arnaud 2019-10-01 15:04:15 UTC
Created attachment 93384 [details] [review]
Bug 20589: Remove expanded_facet variable and fix tests

Thi is a rebase from 14419 to remove an unused variable

Also update tests to expect the new expected result

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 66 Alex Arnaud 2019-10-01 15:06:24 UTC
 - Proposed features works
 - QA tools ok,
 - code ok,
 - work with ES 5.x and 6.x
 - also fix a relevance sorting issue. Great!

Passed QA
Comment 67 Martin Renvoize 2019-10-02 12:49:35 UTC
Nice work!

Pushed to master for 19.11.00
Comment 68 Martin Renvoize 2019-10-02 12:50:16 UTC
Thanks all, great enhancement
Comment 69 David Gustafsson 2019-10-03 12:55:54 UTC
Wonderful, thanks everyone!
Comment 70 Martin Renvoize 2019-10-03 13:01:19 UTC
Hmm, I think it's this one that causing a fair few breaks in tests on master right now.. 

I've narrowed down some of the issues.. for example I don't think we need the Koha::SearchFields->weighted_fields tests at all any more (you've removed the method entirely in your patch but missed this test).

But, there are some more I'm less confident about.. can you guys take a look please?
Comment 71 Martin Renvoize 2019-10-03 13:03:47 UTC
Created attachment 93590 [details] [review]
Bug 20589: (RM follow-up) Fix Koha::SearchField.t

This patch removes the unit test for the weighted_fields method which
this bug removed.
Comment 72 Nick Clemens 2019-10-03 14:42:43 UTC
Created attachment 93624 [details] [review]
Bug 20589: (follow-up) Fix tests
Comment 73 Nick Clemens 2019-10-03 14:44:46 UTC
(In reply to Martin Renvoize from comment #70)
> Hmm, I think it's this one that causing a fair few breaks in tests on master
> right now.. 
> 
> I've narrowed down some of the issues.. for example I don't think we need
> the Koha::SearchFields->weighted_fields tests at all any more (you've
> removed the method entirely in your patch but missed this test).
> 
> But, there are some more I'm less confident about.. can you guys take a look
> please?

I attached a patch, I think the mappings test is probably not the best, but I think we should delete/replace it on its own bug, this gets things running again for now
Comment 74 Martin Renvoize 2019-10-03 16:00:13 UTC
Thanks for the fast followup Nick :) Pushed