https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 Frank Hansen <frank.hansen@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frank.hansen@ub.lu.se -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #1 from Alex Arnaud <alex.arnaud@biblibre.com> --- Created attachment 71607 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71607&action=edit Bug 18969: ES6 - remove use of field [include_in_all] (WIP for biblios only) Test plan: 1) apply this patch, 2) update your elasticsearch server to the version 6 (6.2?), 3) reinstall icu plugin, 4) reindex your authoroties 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 Alex Arnaud <alex.arnaud@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |alex.arnaud@biblibre.com Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Alex Arnaud from comment #3)

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

Is this backwards compatible? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #5 from Alex Arnaud <alex.arnaud@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 Bouzid <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #72654|0 |1 is obsolete| | --- Comment #7 from Bouzid <bouzid.fergani@inlibro.com> --- Created attachment 73498 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=73498&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |glasklas@gmail.com --- Comment #8 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #10 from David Gustafsson <glasklas@gmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #12 from David Gustafsson <glasklas@gmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #14 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #16 from David Gustafsson <glasklas@gmail.com> ---

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/Elastic... 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Elasticsearch - _all field |_all field is deprecated - |is deprecated - should use |should use copy_to to |copy_to to prepare for ES6 |prepare for ES6 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #18 from Alex Arnaud <alex.arnaud@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #20 from Alex Arnaud <alex.arnaud@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- (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?

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #24 from David Gustafsson <glasklas@gmail.com> --- Yes, exactly. :) Waiting for bug_18316 to be merged, and will then refactor and rebase this patch upon the master with 18316 included. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #26 from Alex Arnaud <alex.arnaud@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED CC| |chris@bigballofwax.co.nz --- Comment #28 from Chris Cormack <chris@bigballofwax.co.nz> --- Work for this is being done on bug 20589 and bug 18316 Moving this out of signed off, to save peoples time -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #30 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #31 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #33 from claire.hernandez@biblibre.com <claire.hernandez@biblibre.com> --- Actually, we need a fix that do only ES6 compatibilty. I think we will rebase it anyway :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18969 --- Comment #35 from Alex Arnaud <alex.arnaud@biblibre.com> --- Here is a replacement patch for people that want only ES compatibilty -- You are receiving this mail because: You are watching all bug changes.