Bug 18969 - _all field is deprecated - should use copy_to to prepare for ES6
Summary: _all field is deprecated - should use copy_to to prepare for ES6
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P3 normal (vote)
Assignee: Alex Arnaud
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 20196
  Show dependency treegraph
 
Reported: 2017-07-20 20:11 UTC by Nick Clemens
Modified: 2019-10-02 13:33 UTC (History)
11 users (show)

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


Attachments
Bug 18969: ES6 - remove use of field [include_in_all] (WIP for biblios only) (3.21 KB, patch)
2018-02-14 14:14 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18969: ES6 - replace use of field include_in_all by copy_to (2.87 KB, patch)
2018-02-14 16:04 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18969: ES6 - replace use of field include_in_all by copy_to (3.93 KB, patch)
2018-02-14 16:08 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18969: ES6 - replace use of field include_in_all by copy_to (3.98 KB, patch)
2018-03-12 11:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18969: ES6 - replace use of field include_in_all by copy_to (4.04 KB, patch)
2018-03-30 19:45 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 18969: ES6 - replace use of _all by our own field (2.99 KB, patch)
2019-03-28 16:41 UTC, Alex Arnaud
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-07-20 20:11:45 UTC
Reindex on a clean ES, you get a warning:
[DEPRECATION] 299 Elasticsearch-5.4.1-2cfe0df "field [include_in_all] is deprecated, as [_all] is deprecated, and will be disallowed in 6.0, use [copy_to] instead."
Comment 1 Alex Arnaud 2018-02-14 14:14:24 UTC Comment hidden (obsolete)
Comment 2 Alex Arnaud 2018-02-14 16:04:42 UTC Comment hidden (obsolete)
Comment 3 Alex Arnaud 2018-02-14 16:08:22 UTC
Created attachment 71619 [details] [review]
Bug 18969: ES6 - replace use of field include_in_all by copy_to

Test plan:

  1) apply this patch,
  2) update your elasticsearch server to the version 6 (6.2?),
  3) reinstall icu plugin,
  4) reindex your authorities and biblios,
  5) check that there is no error in
    /var/log/elasticsearch/elasticsearch.log,
  6) try a search on biblios,
  7) check that facet work,
  8) try a search on authorities
Comment 4 Tomás Cohen Arazi 2018-02-26 15:26:36 UTC
(In reply to Alex Arnaud from comment #3)
> Created attachment 71619 [details] [review] [review]
> Bug 18969: ES6 - replace use of field include_in_all by copy_to
> 
> Test plan:
> 
>   1) apply this patch,
>   2) update your elasticsearch server to the version 6 (6.2?),
>   3) reinstall icu plugin,
>   4) reindex your authorities and biblios,
>   5) check that there is no error in
>     /var/log/elasticsearch/elasticsearch.log,
>   6) try a search on biblios,
>   7) check that facet work,
>   8) try a search on authorities

Is this backwards compatible?
Comment 5 Alex Arnaud 2018-02-28 09:47:03 UTC
(In reply to Tomás Cohen Arazi from comment #4)
 
> Is this backwards compatible?

Yep. I tested this patch with ES 5 (5.6.7):
  - Biblio search works (and facets)
  - Authority search work
Comment 6 Nick Clemens 2018-03-12 11:18:54 UTC
Created attachment 72654 [details] [review]
Bug 18969: ES6 - replace use of field include_in_all by copy_to

Test plan:

  1) apply this patch,
  2) update your elasticsearch server to the version 6 (6.2?),
  3) reinstall icu plugin,
  4) reindex your authorities and biblios,
  5) check that there is no error in
    /var/log/elasticsearch/elasticsearch.log,
  6) try a search on biblios,
  7) check that facet work,
  8) try a search on authorities

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Bouzid Fergani 2018-03-30 19:45:47 UTC
Created attachment 73498 [details] [review]
Bug 18969: ES6 - replace use of field include_in_all by copy_to

Test plan:

  1) apply this patch,
  2) update your elasticsearch server to the version 6 (6.2?),
  3) reinstall icu plugin,
  4) reindex your authorities and biblios,
  5) check that there is no error in
    /var/log/elasticsearch/elasticsearch.log,
  6) try a search on biblios,
  7) check that facet work,
  8) try a search on authorities

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 8 David Gustafsson 2018-04-12 09:42:51 UTC
I might be mistaken, I will try to look it up to be certain. But I think when using the query string query in Elasticsearch, the default_field parameter is set to "*.*" as default, so all fields are searched anyway. No need to copy all fields to yet another field, this will also have a negative impact on search performance without improving accuracy, and increase the size of the index. Also, for field boosts to work (title:(title search)^2, each field must be queried individually. A better solution would be to just remove "_all", and even better to produce a relevant list of fields for setting the "fields"-parameter (excluding sortable and facetable fields for example). I will start working on a patch taking this approach, so will probably have a proposal ready quite soon.
Comment 9 Nick Clemens 2018-04-12 10:47:55 UTC
I think you are correct, ES will default to *.*, however, I think having an '_all_fields' and searching that directly gives us a little more flexibility.

Currently fields like 'nonpublicnote' are indexed and searchable, even if hidden from the opac. Using the all field would allow us to keep those fields in the full record stored in ES and search them from the staff side while removing them from the opac side

I believe for boosting we could stil search:
"fields" : ["_all_fields", "title.*^5"],

Thoughts David?
Comment 10 David Gustafsson 2018-04-12 11:49:53 UTC
Hmm.. I'm not really sure I understand. Would not nonpublicnote be searchable as nonpublicnote:"search string", regardless if coming from opac or not? One way of protecting it would be for example to have a different "fields"-list for opac and staff client, and filter out appearances of black-listed fields (nonpublicnote for example) in query string with some regexp in opac, but not staff client?
Comment 11 Nick Clemens 2018-04-12 12:07:58 UTC
(In reply to David Gustafsson from comment #10)
> Hmm.. I'm not really sure I understand. Would not nonpublicnote be
> searchable as nonpublicnote:"search string", regardless if coming from opac
> or not? One way of protecting it would be for example to have a different
> "fields"-list for opac and staff client, and filter out appearances of
> black-listed fields (nonpublicnote for example) in query string with some
> regexp in opac, but not staff client?

Yes, we would need to implement something more along those lines to truly hide it, but having an 'all' would allow for simple keyword searching of everything but the blacklisted fields. This is all theoretical currently, just looking towards possibilities
Comment 12 David Gustafsson 2018-04-12 13:14:37 UTC
Ok, so you mean we would have opac-searchable fields in "_all_fields", and then add additional fields in "fields" for staff client searches? Would it not be better to set "fields" to a list of fields, excluding blacklisted fields, in opac, and all searchable fields in staff client?
Comment 13 Nick Clemens 2018-04-12 13:18:08 UTC
Probably something like that.

'_all_fields' could be default for staff
'_all_opac_fields' could be default for opac, then we catch the blacklisted as you said

Really though, I think this is all a separate bug :-) Mostly I wanted to say that we should keep the copy_to and that it is not a detriment i.e. we should move this patch forward
Comment 14 David Gustafsson 2018-04-12 20:10:12 UTC
Ok! I actually think that would be a bad idea, mainly for the following reasons:

1) Elasticsearch uses ranking function called Okapi BM25 (used to be Term Frequency/Inverse Document Frequency (TF/IDF), which similar but simpler to understand). Two of the parameters Okapi BM25 uses to calculate the relevancy score (per field) are average field length and inverse document frequency (IDF). If you put all values in one field, average field length and inverse document frequency will averaged out based on all fields, effectively crippling the algorithm rendering it unable to calculate relevancy properly.

2) You will also not be able to use per field boosting, unless you add boosted fields to "fields" as well, but then you might as well skip the "_all_*" fields and pass along the full list of fields instead.

3) The index will be about 3x as big, increasing memory usage. This might not a huge issue, but could be for us for example as we have several million biblios and already quite a large index already.

4) To utilize the full power of Elasticsearch one would want to be able to use different analyzers/normalizers and other useful mapping settings on a per field basis, and nice query string query options like "quote_field_suffix". With everyting in one field, all data will be indexed using the same mapping settings, and features like quote_field_suffix will not work.

I can actually see no benefits with using "all_*" fields, and no real downside by instead generating a proper "fields" containing all searchable fields. I begun working on a patch today (one of the reasons was that we need per field boosting), and it's actually not a very complicated change. Might not be ready tomorrow, but at least some time in the beginning of next week.
Comment 15 Nick Clemens 2018-04-16 15:05:41 UTC
(In reply to David Gustafsson from comment #14)
> Ok! I actually think that would be a bad idea, mainly for the following
> reasons:
> 
> 1) Elasticsearch uses ranking function called Okapi BM25...If you put all values in one field, average field length
> and inverse document frequency will averaged out based on all fields,

Ah, okay, I see this in the documentation:
https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-all-field.html

Built-in or constructed we pay a relevance price


> 2) You will also not be able to use per field boosting, unless you add
> boosted fields to "fields" as well, but then you might as well skip the
> "_all_*" fields and pass along the full list of fields instead.

Well, it does seem to work to only add boosted fields and boost those above all, but again, not as exact

> 3) The index will be about 3x as big, increasing memory usage. This might
> not a huge issue, but could be for us for example as we have several million
> biblios and already quite a large index already.

Agreed, I think we would need to compare with an without the all field to see exact impact


> 4) To utilize the full power of Elasticsearch one would want to be able to
> use different analyzers/normalizers and other useful mapping settings on a
> per field basis, and nice query string query options like
> "quote_field_suffix". With everything in one field, all data will be indexed
> using the same mapping settings, and features like quote_field_suffix will
> not work.

I don't think I actually follow you here - we still specify different analyzers per field, but we also construct the _all field and use that for keyword searching only - this is what we currently do. So we can search specific fields, or use the all



> I can actually see no benefits with using "all_*" fields, and no real
> downside by instead generating a proper "fields" containing all searchable
> fields. 
The only downside is listing all the fields individually so a small cost in construction of queries and query size, but not terrible I would think

>I begun working on a patch today (one of the reasons was that we
> need per field boosting), and it's actually not a very complicated change.
> Might not be ready tomorrow, but at least some time in the beginning of next
> week.

Looking forward to it! :-) - have you seen bug 18316? https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18316
Comment 16 David Gustafsson 2018-04-16 18:12:55 UTC
> I don't think I actually follow you here - we still specify different analyzers
> per field, but we also construct the _all field and use that for keyword
> searching only - this is what we currently do. So we can search specific
> fields, or use the all.

My point was with regards to this line: https://github.com/Koha-Community/Koha/blob/master/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm#L93
where default_field is set to '_all', and in the patch this is changed to '_all_fields'. If searching for some terms (without specifying specific fields in the search string), the default_field will be used, 'all_fields' in this case, and no field-specific analyzers will be used. If instead setting "fields" to all relevant fields, the field-specific analyzers etc will be applied.

> The only downside is listing all the fields individually so a small cost in
> construction of queries and query size, but not terrible I would think

Yes, you are correct. The query construction overhead is insignificant, but it will incur a slightly higher cost executing the query as multiple fields will be queried instead of just one. But I still don't see using just one field as an option since this would make most of the nice Elasticsearch free text search features impossible to implement.


> Looking forward to it! :-) - have you seen bug 18316?
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18316

Damn, must have missed that one. They are still slightly incompatible so perhaps still would have needed to re-implement field boosts (as the new patch always uses the "field" parameter with possible boosts, not just for boosted fields).

I created a new issue for my suggestion regarding remove the "_all" field and field boosts: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20589
Comment 17 Katrin Fischer 2018-07-08 11:07:23 UTC
What should be the next step here? I am bit lost in the discussion, has consensus be reached or should we move this out of the queue for more discussion?
Comment 18 Alex Arnaud 2018-07-09 09:13:13 UTC
(In reply to Katrin Fischer from comment #17)
> What should be the next step here? I am bit lost in the discussion, has
> consensus be reached or should we move this out of the queue for more
> discussion?

If i understand correctly, storing the entire record in all_fields_* would help us to hide some fields like nonpublicnote (Nick, another advantage?) by creating all_fields and all_fields_opac. But this could be done with a staff/opac parameter on ES configuration form like David did in bug 20589. 

I wrote this patch in order to make Koha compliant with ES 6. But if we remove this "feature", it is the same result. And the job is also done in bug 20589.
Comment 19 Katrin Fischer 2018-07-09 09:52:46 UTC
> If i understand correctly, storing the entire record in all_fields_* would
> help us to hide some fields like nonpublicnote (Nick, another advantage?) by
> creating all_fields and all_fields_opac. But this could be done with a
> staff/opac parameter on ES configuration form like David did in bug 20589. 
> 
> I wrote this patch in order to make Koha compliant with ES 6. But if we
> remove this "feature", it is the same result. And the job is also done in
> bug 20589.

Which job does 20589? Resolving the ES6 issue?
Comment 20 Alex Arnaud 2018-07-09 10:05:10 UTC
(In reply to Katrin Fischer from comment #19)
> > If i understand correctly, storing the entire record in all_fields_* would
> > help us to hide some fields like nonpublicnote (Nick, another advantage?) by
> > creating all_fields and all_fields_opac. But this could be done with a
> > staff/opac parameter on ES configuration form like David did in bug 20589. 
> > 
> > I wrote this patch in order to make Koha compliant with ES 6. But if we
> > remove this "feature", it is the same result. And the job is also done in
> > bug 20589.
> 
> Which job does 20589? Resolving the ES6 issue?

Removing deprecated _all
Comment 21 Katrin Fischer 2018-07-09 10:23:11 UTC
Trying to unravel these Elasticsearch patches gives me a headache. ;)

What would be your "next steps" - should we drop this patch in favour of 20589? 

So in order to 'unlock' bug 20589 we'd have to qa bug 18316, correct?
Comment 22 Nick Clemens 2018-07-09 13:37:44 UTC
(In reply to Katrin Fischer from comment #21)
> What would be your "next steps" - should we drop this patch in favour of
> 20589? 

If Alex agrees then yes, we should take David's approach

> So in order to 'unlock' bug 20589 we'd have to qa bug 18316, correct?
Yes, please :-) It looks like it had passed QA and was failed by Joubu on RM review, perhaps he could take a second look now?
Comment 23 Alex Arnaud 2018-07-09 13:50:41 UTC
(In reply to Katrin Fischer from comment #21)
> Trying to unravel these Elasticsearch patches gives me a headache. ;)
> 
> What would be your "next steps" - should we drop this patch in favour of
> 20589?
Yes! I'm ok for giving up on this one and working on bug 20589.
> 
> So in order to 'unlock' bug 20589 we'd have to qa bug 18316, correct?

I think since David took part on bug 18316 (boosting feature), he should provide a new patch on 20589 without boosting (the 2 patch conflicts). 

On 20589, we need: removing deprecated _all + staff/opac options for a field

David, are you ok?
Comment 24 David Gustafsson 2018-07-09 14:43:29 UTC
Yes, exactly. :) Waiting for bug_18316 to be merged, and will then refactor and rebase this patch upon the master with 18316 included.
Comment 25 David Gustafsson 2018-07-09 14:44:15 UTC
bug 18316
Comment 26 Alex Arnaud 2018-07-10 07:56:57 UTC
(In reply to David Gustafsson from comment #24)
> Yes, exactly. :) Waiting for bug_18316 to be merged, and will then refactor
> and rebase this patch upon the master with 18316 included.

You can build bug 20589 upon 18316 and make it depend on it without waiting for it to be pushed.
Comment 27 David Gustafsson 2018-07-10 11:19:56 UTC
Yes I could do that, but would still rather wait until bug 20589 is merged since that would mean not having to perform further rebasing/refactoring if more changes introduced in 20589.
Comment 28 Chris Cormack 2018-07-11 01:42:45 UTC
Work for this is being done on bug 20589 and bug 18316

Moving this out of signed off, to save peoples time
Comment 29 claire.hernandez@biblibre.com 2019-03-27 10:57:41 UTC
We really need this patch. It does only what it must do. One bugzilla should do one thing and this one is a good candidate...
Comment 30 David Gustafsson 2019-03-27 11:59:50 UTC
I still think bug 20589 is the way to go instead of this one for the reasons listed above. There is a reason the _all field is deprictated and recreating it only postpones solving the real underlying problem.
Comment 31 Nick Clemens 2019-03-27 12:46:44 UTC
(In reply to David Gustafsson from comment #30)
> I still think bug 20589 is the way to go instead of this one for the reasons
> listed above. There is a reason the _all field is deprictated and recreating
> it only postpones solving the real underlying problem.

I added Ere on both of these bugs - I do think 20589 might be the better path forward. After working with a library in production it does seem that searching/relevancy works better when the fields used for searching are limited
Comment 32 Ere Maijala 2019-03-27 13:15:08 UTC
I agree that bug 20589 is the proper solution. Note that at least in master include_in_all is no longer used, so the patch here is obsolete. There may be something else not in master yet, but we need at least bug 20589 to be able to configure the ES client version to use.
Comment 33 claire.hernandez@biblibre.com 2019-03-28 14:00:00 UTC
Actually, we need a fix that do only ES6 compatibilty. I think we will rebase it anyway :)
Comment 34 Alex Arnaud 2019-03-28 16:41:22 UTC
Created attachment 87143 [details] [review]
Bug 18969: ES6 - replace use of _all by our own field
Comment 35 Alex Arnaud 2019-03-28 16:42:52 UTC
Here is a replacement patch for people that want only ES compatibilty
Comment 36 Alex Sassmannshausen 2019-10-02 13:33:56 UTC
It was generally agreed that bug20589 renders this patch obsolete.

bug20589 is now in master, so we can close this bug.

Closing after checking with Alex & Nick.