Created attachment 61472 [details] [review] Bug 18316 - Add weighting/relevancy options to ElasticSearch These are proof of concept that we can add weighting simply at time of search query. Testable but not suitable for pushing. To test: Apply patch Use the advanced search page in the intranet Choose a field and enter a number to weight Note diffrently ordered results than without any weighing To DO: 1 - Add a config page for default wieghting to be used 2 - Allow addition of any indexed field to wieghting at search 3 - All more than one fields to be weighted 4 - Better code/variables
Created attachment 73139 [details] [review] Bug 18316 - Add weighting/relevancy options to ElasticSearch These are proof of concept that we can add weighting simply at time of search query. Testable but not suitable for pushing. To test: Apply patch Use the advanced search page in the intranet Choose a field and enter a number to weight Note diffrently ordered results than without any weighing To DO: 1 - Add a config page for default wieghting to be used 2 - Allow addition of any indexed field to wieghting at search 3 - All more than one fields to be weighted 4 - Better code/variables
Created attachment 73140 [details] [review] Bug 18316 - Add weighting/relevancy options to ElasticSearch These are proof of concept that we can add weighting simply at time of search query. Testable but not suitable for pushing. To test: Apply patch Use the advanced search page in the intranet Choose a field and enter a number to weight Note diffrently ordered results than without any weighing To DO: 1 - Add a config page for default wieghting to be used 2 - Allow addition of any indexed field to wieghting at search 3 - All more than one fields to be weighted 4 - Better code/variables Rebased-by Alex Arnaud <alex.arnaud@biblibre.com> (2018-03-21)
Alex is working on this. We are assuming relevancy working is mostly important for simple search, or what zebra calls the "keyword" index. What we plan to do: Add columns to the "search fields" tab of the search engine configuration page, where the user will be able to indicate a weight for each field. This will allow to configure the way the simple search is weighted: it will search in all fields for which a weight has been indicated. If no weight was indicated it will search in all fields. (In addition, this allows excluding some search fields from the simple search!) This will not affect advanced search, in which the user can pick indices himself. (But it could, and we would then run the query with the weight defined for each field.) The possibility to add a weight to a search field will only be offered if this field is used in the biblio mapping. So it won't be possible to give weight to a field only used in the authorities mapping, or defined but unused. In the future it should be possible to have "koha indices" that search across multiple elasticsearch fields, each with a different weight. But this seems a bit ambitious for now, especially with 18.05 so close. So this seems like a reasonable goal for the next release, and certainly a very useful one.
This sounds like a reasonable approach, looking forward to seeing it :-)
Created attachment 73492 [details] [review] Bug 18316 - Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset
Created attachment 73493 [details] [review] Bug 18316 - Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields
Created attachment 73600 [details] [review] Bug 18316 - Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 73601 [details] [review] Bug 18316 - Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Patchs work as described. I misunderstood the way to use numbers, thinking "1" was the most important field(s) I wanted to search in. Maybe a warning or more detailed description would be useful for reindex can take time for big databases, it would be sad if others make the same mistake as I. This weighting add a lot of relevance to the simple search. In reaction to Gaetan's comment, it also appears that having weight on sub-fields used on the same index would be great. For example, when searching with index "title" in our Unimarc system, we would like to display books that contains the searched word in 200$b before the ones in 410$t.
(In reply to Séverine Queune from comment #10) > Patchs work as described. > > I misunderstood the way to use numbers, thinking "1" was the most important > field(s) I wanted to search in. > Maybe a warning or more detailed description would be useful for reindex can > take time for big databases, it would be sad if others make the same mistake > as I. You mean add a warning to tell users that more the weight is hight more the field is important? > > This weighting add a lot of relevance to the simple search. > > In reaction to Gaetan's comment, it also appears that having weight on > sub-fields used on the same index would be great. > For example, when searching with index "title" in our Unimarc system, we > would like to display books that contains the searched word in 200$b before > the ones in 410$t. I'm not yet confortable enough with all ES capabilities, but it could be done by adding a "other-title" search field with a different weight. Does it make sens for you?
(In reply to Alex Arnaud from comment #11) > (In reply to Séverine Queune from comment #10) > > Patchs work as described. > > > > I misunderstood the way to use numbers, thinking "1" was the most important > > field(s) I wanted to search in. > > Maybe a warning or more detailed description would be useful for reindex can > > take time for big databases, it would be sad if others make the same mistake > > as I. > You mean add a warning to tell users that more the weight is hight more the > field is important? I think that's what she meant. > > This weighting add a lot of relevance to the simple search. > > > > In reaction to Gaetan's comment, it also appears that having weight on > > sub-fields used on the same index would be great. > > For example, when searching with index "title" in our Unimarc system, we > > would like to display books that contains the searched word in 200$b before > > the ones in 410$t. > I'm not yet confortable enough with all ES capabilities, but it could be > done by adding a "other-title" search field with a different weight. Does it > make sens for you? Yes, but not to our librarians colleagues who want to describe titles in a myriad of MARC' subfield while uniting them all under one index entry. I'm not sure to be able to convince them this may not be a good idea :). The 4XX$t as a title index for instance, is a relevancy killer. I'll try to convince them to bind it to a linked-with-title index. Anyway, having a different relevancy for same index/different subfield may be another bug.
I agree - different weighting for specific fields in an index would be a separate bug Searching and weighting works well and tests pass I have questions about the comments: >Weight: define weight between 1 and 99. add: Higher numbers indicate increased relevenacy >search will be done on weighted fields only this doesn't seem to be the case - weight author - search for an isbn, you get the record >if no field is weighted, search will be done on all the record true, but we search all fields even with some wieghts I think the behaviour is correct, searching _all and adding weighted fields, just need to adjust comments Should we indicate that weights are not used during adv search? (If we want them I think that can be done on a future patch)
(In reply to Nick Clemens from comment #13) > I agree - different weighting for specific fields in an index would be a > separate bug +1 > > Searching and weighting works well and tests pass > > I have questions about the comments: > > >Weight: define weight between 1 and 99. > add: Higher numbers indicate increased relevenacy > > >search will be done on weighted fields only > this doesn't seem to be the case - weight author - search for an isbn, you > get the record Right. Search is done on all fields but boost the weighted ones. > > >if no field is weighted, search will be done on all the record > true, but we search all fields even with some wieghts > > I think the behaviour is correct, searching _all and adding weighted fields, > just need to adjust comments > > Should we indicate that weights are not used during adv search? (If we want > them I think that can be done on a future patch) I can do it. The future patch for adv search will have to remove the comment.
Created attachment 74112 [details] [review] Bug 18316 - String changes
Created attachment 74115 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 74116 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 74117 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
1. From Koha::SearchFields->weighted_fields + while ( my $field = $fields->next ) { + push @$w_fields, $field->name; + push @$weight, $field->weight; + } + + return ($w_fields, $weight); From search.pl: ($w_fields, $weight) = Koha::SearchFields->weighted_fields(); then we call build_query_compat with { w_fields => @$w_fields, weight => @$weight } which will finally do: + if ( defined $weights[0] ) { + for (my $i = 0 ; $i < (scalar @weights) ; $i++ ){ + push @fields, "$w_fields[$i]^$weights[$i]"; + } + } so basically you want to fetch the weighted fields from build_query_compat, with something like: push @fields, sprintf("%s^%s", $_->name, $_->weight) for Koha::SearchFields->weighted_fields; That seems much more easier, what did I miss? 2. Sounds like you could replaced the loop in is_mapped_biblios, with $self->search_marc_maps->search({ index_name => 'biblios' })->count ? 1 : 0; 3. search_marc_maps reads wrong too.
(In reply to Jonathan Druart from comment #19) > 1. > From Koha::SearchFields->weighted_fields > > + while ( my $field = $fields->next ) { > + push @$w_fields, $field->name; > + push @$weight, $field->weight; > + } > + > + return ($w_fields, $weight); > > From search.pl: > ($w_fields, $weight) = Koha::SearchFields->weighted_fields(); > > then we call build_query_compat with { w_fields => @$w_fields, weight => > @$weight } > > which will finally do: > + if ( defined $weights[0] ) { > + for (my $i = 0 ; $i < (scalar @weights) ; $i++ ){ > + push @fields, "$w_fields[$i]^$weights[$i]"; > + } > + } > > so basically you want to fetch the weighted fields from build_query_compat, > with something like: > push @fields, sprintf("%s^%s", $_->name, $_->weight) for > Koha::SearchFields->weighted_fields; > > That seems much more easier, what did I miss? We don't want to use weighted fields on advanced search and build_query_compat is called for both simple and adv search. But i can propose something similar in search.pl > > > 2. Sounds like you could replaced the loop in is_mapped_biblios, with > $self->search_marc_maps->search({ index_name => 'biblios' })->count ? 1 : 0; Right. > > 3. search_marc_maps reads wrong too. What is wrong?
Created attachment 74714 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set
Created attachment 74720 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set
Created attachment 74751 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74752 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74753 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74754 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Tested with chinese, persian and latin (english/french) languages. Relevance is the same as before (with the first patch) so good for me !
Nice work. I was unable to apply patch using git bz apply, getting: error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm). error: could not build fake ancestor Am I doing something wrong or is there an issue with the current patches? I also have some questions/suggestions: 1) I find it slightly confusing that "weight" is in the patch whilst "boost" is used in elastic. Not a big deal at all, but would be nicer to use the same terminology as Elastic imho. 2) Weight/boost should be decimal, for example DECIMAL(5,2) should be more than enough precision. 3) Why not use field boosting in advanced search? As a user I would find it confusing to get different results performing the same search in both interfaces. 4) It seems like field boosts/weights specified in mappings.yaml will not be picked up, this could be added in reset_elasticsearch_mappings in Elasticsearch.pm. I can contribute patches for some or all of the above s depending on which ones are deemed good suggestions once I am able to apply the current patch.
Hi David, please change to 'Patch doesn't apply' in this case. Some devs use saved searches to see where they need to take action, this makes it easier.
(In reply to David Gustafsson from comment #28) > Nice work. I was unable to apply patch using git bz apply, getting: > > error: sha1 information is lacking or useless > (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm). > error: could not build fake ancestor > > Am I doing something wrong or is there an issue with the current patches? Fixed! > > I also have some questions/suggestions: > > 1) I find it slightly confusing that "weight" is in the patch whilst "boost" > is used in elastic. Not a big deal at all, but would be nicer to use the > same terminology as Elastic imho. I'm not against renaming it. In my mind, boost is most seen as a yes/not choice (boosted/not boosted) and not as range of values. > > 2) Weight/boost should be decimal, for example DECIMAL(5,2) should be more > than enough precision. Currently we can set it between 1 and 99. Do you think we need more precision? > > 3) Why not use field boosting in advanced search? As a user I would find it > confusing to get different results performing the same search in both > interfaces. We plan to do that later. We believed that adv search allows to search on specific(s) field(s). So boosting seems less relevant. > > 4) It seems like field boosts/weights specified in mappings.yaml will not be > picked up, this could be added in reset_elasticsearch_mappings in > Elasticsearch.pm. See bug 20607 which depend on this one > > I can contribute patches for some or all of the above s depending on which > ones are deemed good suggestions once I am able to apply the current patch.
Created attachment 75073 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75074 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75075 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75076 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Sorry about the late reply. > Fixed! Great :) > I'm not against renaming it. In my mind, boost is most seen as a yes/not > choice (boosted/not boosted) and not as range of values. As previously noted, not a big deal, my main issue with it is that boost is used in lucene/elastic. > Currently we can set it between 1 and 99. Do you think we need more precision? In lucene/elastic weight is a decimal number. So I really think it should be a decimal for that reason. The admin interface should then probably use text inputs instead of selects with predefined values. There are also situations where you just want to adjust the relevancy a very small amount to make sure queries are perfectly balanced and that one does not always dominate the relevance scoring. With integers this is much more difficult, and you will be forced to assign boosts to many more fields to get the increased precision. > We plan to do that later. We believed that adv search allows to search on > specific(s) field(s). So boosting seems less relevant. Ok, I adjusted some of the queries for authorities search to utilize fields better in bug 20589. So perhaps I can include the field boots if I rebase that patch on this one when it has been merged in Koha. > See bug 20607 which depend on this one Aha, ok.
Oops, regarding my response to: > We plan to do that later. We believed that adv search allows to search on > specific(s) field(s). So boosting seems less relevant. I mixed it up with authorities search. So to clarify, I think boosts should be used also for authorities search (although not that important), but agree it is not obvious that boosts should be applied in advanced search.
Created attachment 75458 [details] [review] Change search field weight field to decimal and add column declaration to DBIx results source
(In reply to David Gustafsson from comment #37) > Created attachment 75458 [details] [review] [review] > Change search field weight field to decimal and add column declaration to > DBIx results source Hello David, You provide changes on Koha/Schema/Result/SearchField.pm. This should be done in a dedicated patch. RM will ignore it. Also, you replaced the input type number with a input text. This cause errors when typing non-numeric characters in. You can use a input number with step=0.1 to increment decimal numbers. Need to fail QA
(In reply to Alex Arnaud from comment #38) > (In reply to David Gustafsson from comment #37) > > Created attachment 75458 [details] [review] [review] [review] > > Change search field weight field to decimal and add column declaration to > > DBIx results source > > Hello David, > > You provide changes on Koha/Schema/Result/SearchField.pm. This should be > done in a dedicated patch. RM will ignore it. > > Also, you replaced the input type number with a input text. This cause > errors when typing non-numeric characters in. You can use a input number > with step=0.1 to increment decimal numbers. > > Need to fail QA Input number for decimals can be problematic for translations in my experience. Please test for example with de-DE if the value is correctly stored (it will force a decimal comma instead of . on input, which is inconsistent with our other input fields right now)
(In reply to Alex Arnaud from comment #38) > (In reply to David Gustafsson from comment #37) > > Created attachment 75458 [details] [review] [review] [review] > > Change search field weight field to decimal and add column declaration to > > DBIx results source > > Hello David, > > You provide changes on Koha/Schema/Result/SearchField.pm. This should be > done in a dedicated patch. RM will ignore it. > > Also, you replaced the input type number with a input text. This cause > errors when typing non-numeric characters in. You can use a input number > with step=0.1 to increment decimal numbers. > > Need to fail QA Ok, I can fix this, though precision should be 0.01 in that case. There is server side validation, so everything that is not a number will be ignored regardless of input type. I was not aware that Koha/Schema/Result/* files should not be changed. How does that work for signing off, since without the change Koha will throw an error when trying to save weights in admin UI?
(In reply to David Gustafsson from comment #40) > (In reply to Alex Arnaud from comment #38) > > (In reply to David Gustafsson from comment #37) > > > Created attachment 75458 [details] [review] [review] [review] [review] > > > Change search field weight field to decimal and add column declaration to > > > DBIx results source > > > > Hello David, > > > > You provide changes on Koha/Schema/Result/SearchField.pm. This should be > > done in a dedicated patch. RM will ignore it. > > > > Also, you replaced the input type number with a input text. This cause > > errors when typing non-numeric characters in. You can use a input number > > with step=0.1 to increment decimal numbers. > > > > Need to fail QA > > Ok, I can fix this, though precision should be 0.01 in that case. There is > server side validation, so everything that is not a number will be ignored > regardless of input type. > > I was not aware that Koha/Schema/Result/* files should not be changed. How > does that work for signing off, since without the change Koha will throw an > error when trying to save weights in admin UI? Generally you make your code changes, commit the patches, then run: misc/devel/update_dbix_class_files.pl And commit those as their own patch - that way the changes are included for testers but can be regenerated when pushing to master
(In reply to Katrin Fischer from comment #39) > Input number for decimals can be problematic for translations in my > experience. Please test for example with de-DE if the value is correctly > stored (it will force a decimal comma instead of . on input, which is > inconsistent with our other input fields right now) I just tried on a Koha translated into German. Sounds that there no problem with the values stored. Any test plan to reproduice issue you talked about?
Created attachment 75490 [details] [review] Update Koha/Schema/Result/SearchField.pm
(In reply to Katrin Fischer from comment #39) > (In reply to Alex Arnaud from comment #38) > > (In reply to David Gustafsson from comment #37) > > > Created attachment 75458 [details] [review] [review] [review] [review] > > > Change search field weight field to decimal and add column declaration to > > > DBIx results source > > > > Hello David, > > > > You provide changes on Koha/Schema/Result/SearchField.pm. This should be > > done in a dedicated patch. RM will ignore it. > > > > Also, you replaced the input type number with a input text. This cause > > errors when typing non-numeric characters in. You can use a input number > > with step=0.1 to increment decimal numbers. > > > > Need to fail QA > > Input number for decimals can be problematic for translations in my > experience. Please test for example with de-DE if the value is correctly > stored (it will force a decimal comma instead of . on input, which is > inconsistent with our other input fields right now) Now updated the patch with requested changes. Also tried with German translation and worked for me.
(In reply to Nick Clemens from comment #41) > Generally you make your code changes, commit the patches, then run: > misc/devel/update_dbix_class_files.pl > And commit those as their own patch - that way the changes are included for > testers but can be regenerated when pushing to master Thanks for clarifying this, now I understand how to go about it.
(In reply to Alex Arnaud from comment #42) > (In reply to Katrin Fischer from comment #39) > > > Input number for decimals can be problematic for translations in my > > experience. Please test for example with de-DE if the value is correctly > > stored (it will force a decimal comma instead of . on input, which is > > inconsistent with our other input fields right now) > > I just tried on a Koha translated into German. Sounds that there no problem > with the values stored. Any test plan to reproduice issue you talked about? The problem we had was that it forced you to enter decimal values with comma but Koha didn't undertand them and then truncated the value. 1,1 = 1. This happened in fines in an older version, some more info here: Bug 17098 - Validation problems with form field type "number" (decimal separator) We are still using the patch described there. It might depend on the browser and version you use and will only appear if the template language is one that uses decimal commas. But what remains, I think, is a problem of consistency. Having some inputs ask for comma and others for . is confusing to libraries.
(In reply to David Gustafsson from comment #43) > Created attachment 75490 [details] [review] [review] > Update Koha/Schema/Result/SearchField.pm Hello David, Your dedicated patch is ok. But, please, remove the changes on the schema file from the previous one: attachment 75458 [details] [review]. And add the bug number at the start of your commit messages.
Created attachment 75519 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Created attachment 75520 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Created attachment 75521 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75522 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75523 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75524 [details] [review] Bug 18316: Change search field weight field to decimal
Created attachment 75525 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Created attachment 75526 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75527 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75528 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75529 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75530 [details] [review] Bug 18316: Change search field weight field to decimal
Created attachment 75531 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Not used to working with multiple patches in git bz, so had removed the schema changes locally, but used git bz incorrectly so it did not get included. Later managed to screw things up even more, but now managed to restore the patches (which I accidentally obsoleted) and fix the two last ones.
The sandbox you've requested is not ready. Some problems occurred applying patches from bug 18316: <h1>Something went wrong !</h1>Applying: Bug 18316: Ability to weight search fields Using index info to reconstruct a base tree... M installer/data/mysql/kohastructure.sql M koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql error: Failed to merge in the changes. Patch failed at 0001 Bug 18316: Ability to weight search fields The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. Bug 18316 - Add weighting/relevancy options to ElasticSearch 75526 - Bug 18316: Ability to weight search fields 75527 - Bug 18316: Add weighting/relevancy options to ES query on simple search 75528 - Bug 18316: String changes 75529 - Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set 75530 - Bug 18316: Change search field weight field to decimal 75531 - Bug 18316: Update Koha/Schema/Result/SearchField.pm Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-18316-Ability-to-weight-search-fields-MFIcJl.patch . Same result on my test instance :/
I think root cause of this is that the "Bug 18316: Ability to weight search fields" commit was created from a commit with the line: "type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum', 'isbn', 'stdno') ..." in /data/mysql/kohastructure.sql when in current Koha master the line is: `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum') This is the reason why: https://github.com/Koha-Community/Koha/commit/cff62796ea9fc227c24fec8f8f3f153a715796ef I can rebase the patch.
Created attachment 75536 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75537 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75538 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75539 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 75540 [details] [review] Bug 18316: Change search field weight field to decimal
Created attachment 75541 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Now patches should apply to latest master.
Forgot to change status.
Sorry David, I took a few days off and didn't try this patch earlier, it doesn't apply on last master... Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 18316: Ability to weight search fields Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: The following untracked working tree files would be overwritten by merge: installer/data/mysql/atomicupdate/bug_18316_add-weight-column.perl t/db_dependent/Koha/SearchField.t Please move or remove them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 Bug 18316: Ability to weight search fields
(In reply to Séverine Queune from comment #72) > error: The following untracked working tree files would be overwritten by > merge: > installer/data/mysql/atomicupdate/bug_18316_add-weight-column.perl > t/db_dependent/Koha/SearchField.t This error means you have these files in your repo and the patch is trying to create it. You should remove them, then apply the patch. However there is another conflict later: CONFLICT (content): Merge conflict in Koha/Schema/Result/SearchField.pm
Hmm, thought I had fixed the merge conflicts, but perhaps I was using a non up to date or something has changed since last time. Can have a look at it tomorrow if no one else does before then.
Created attachment 75699 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Yes, work for the master I did the rebase on, but the signature comment in SearchField.pm had changed since then. Now should work again.
The patch "Bug 18316: Change search field weight field to decimal" seems to break the ability to erase weights. I.e - set weihgt to 1.25 for title, - save, - erase (empty) the weight input for title, - save, - weight is still here with the same value.
Created attachment 75730 [details] [review] Bug 18316: Fix field weight validation/deletion bug
Ok! This should now be fixed.
I am sorry, I fixed the conflict in kohastructure.sql, but then the 4th patch won't apply: sloth:/usr/local/vagrant/kohaclone_master (32-18316-qa-elastic_weight|AM 1/1) $ git bz apply --continue Applying: Bug 18316: Ability to weight search fields Applying: Bug 18316: Add weighting/relevancy options to ES query on simple search Applying: Bug 18316: String changes Applying: Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm). error: could not build fake ancestor Patch failed at 0001 Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-18316---QA-improvement---KohaSearchFieldsearch-dKxuaY.patch
Created attachment 77661 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 77662 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on these fields only and with the coresponding weights. Else, search is done on the entire record. Test plan (having Koha working with Elasticsearch): - apply this patch, - try searches with and without weight defined on search fields Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 77663 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 77664 [details] [review] Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 77665 [details] [review] Bug 18316: Change search field weight field to decimal
Created attachment 77666 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Created attachment 77667 [details] [review] Bug 18316: Fix field weight validation/deletion bug
Giving this a go, any help/more eyes much appreaciated! 1) Questions 1.1) >This will allow to configure the way the simple search is weighted: >it will search in all fields for which a weight has been indicated. >If no weight was indicated it will search in all fields. (In addition, >this allows excluding some search fields from the simple search!) From comments#13+ I tihnk this is no longer the case and it will be always searched on all fields + boosting the weighted fields. Can you confirm? 1.2) Should the patches on this bug tested in combination with other patches from dependent bugs? A more detailed test plan would help me a lot. 1.3) Could we deliver this with a basic default weighting? 2) QA tools and unit tests 2.1) FAIL t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid You tried to plan twice 2.2) When I remove the doubled up line, the test still fails: t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 2/2 subtests Test Summary Report ------------------- t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t (Wstat: 65280 Tests: 0 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 2 tests but ran 0. Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.23 cusr 0.05 csys = 0.30 CPU) Result: FAIL kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ vi t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t .. 1/3 # No tests run! # Failed test 'No tests run for subtest "build_query with weighted fields tests"' # at t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t line 167. Can't locate object method "new" via package "t::lib::TestBuilder" (perhaps you forgot to load "t::lib::TestBuilder"?) at t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t line 127. # Looks like your test exited with 255 just after 3. t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 1/3 subtests 2.3) * Commit title does not start with 'Bug XXXXX: ' - dfe2ab9 2.4) Other tests fail too: t/db_dependent/Koha/SearchField.t .. 1/15 # Failed test 'Title weight is 25.' # at t/db_dependent/Koha/SearchField.t line 196. # got: '25.00' # expected: '25' # Failed test 'Subject weight is 15.' # at t/db_dependent/Koha/SearchField.t line 198. # got: '15.00' # expected: '15' # Failed test 'Author weight is 5.' # at t/db_dependent/Koha/SearchField.t line 200. # got: '5.00' # expected: '5' # Looks like you failed 3 tests of 15. t/db_dependent/Koha/SearchField.t .. Dubious, test returned 3 (wstat 768, 0x300) Failed 3/15 subtests 3) Code Review 3.1) Please use AFTER/BEFORE to make sure the column ends always in the correct spot. + $dbh->do( "ALTER TABLE `search_field` ADD COLUMN `weight` decimal(5,2) DEFAULT NULL" ); 3.2) Can you explain this line? <input type="hidden" name="advsearch" value="1"/> 4) Further tests Adding, deleting and editing weights works. Waiting for follow-up and more info before I continue!
(In reply to Katrin Fischer from comment #88) > 3.2) Can you explain this line? > <input type="hidden" name="advsearch" value="1"/> It is used in search.pl to enable weighted field search to any other search expect the one done in the Advanced search page (/cgi-bin/koha/catalogue/search.pl): unless ( $cgi->param('advsearch') ) { $build_params->{weighted_fields} = 1; } So if the advsearch parameter equals '1' the weighted search is disabled.
Instead of adding a new 'advsearch' parameter maybe checking whether the 'idx' parameter is present would work just fine?
Thx, Joonas. Waiting for a follow-up here so I can continue testing - hopefully soon!
We had agreed that this bug should be next, still waiting, please fix!
Created attachment 78396 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 78397 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on all fields plus those with the coresponding weights. Else, search is done on the entire record with no weighting. The advanced search page is unaffected by these changes Test plan (having Koha working with Elasticsearch): - apply this patch - have some weights defined for various fields - try searches from the search bar and from the advanced search page - confirm weighting affects the relevancy (in expected ways) e.g. 1. search for 'a' from advanced search, note results 2. give 'title' a weight 3. search for 'a' using the simple search bar 4. results with 'a' in the title should now be more relevant - confirm search results on advanced search page are unaffected Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 78398 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 78399 [details] [review] Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 78400 [details] [review] Bug 18316: Change search field weight field to decimal
Created attachment 78401 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm
Created attachment 78402 [details] [review] Bug 18316: Fix field weight validation/deletion bug
Created attachment 78403 [details] [review] Bug 18316: (follow-up) Fix tests and update statement
(In reply to Joonas Kylmälä from comment #90) > Instead of adding a new 'advsearch' parameter maybe checking whether the > 'idx' parameter is present would work just fine? I think having the search parameter to weight or not could be useful - advsearch should get weighting in future but may offer specific weighting. We also may want the ability to sometimes search without weights
Comment on attachment 78397 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search Review of attachment 78397 [details] [review]: ----------------------------------------------------------------- ::: Koha/SearchEngine/Elasticsearch/QueryBuilder.pm @@ +92,5 @@ > default_operator => 'AND', > default_field => '_all', > lenient => JSON::true, > + fields => $options{fields}, > + use_dis_max => JSON::true, From https://www.elastic.co/guide/en/elasticsearch/reference/6.0/breaking_60_search_changes.html > The use_dis_max parameter for the query_string query has been removed. If > provided, it will be ignored and issue a deprecation warning. The tie_breaker > parameter must be used instead. In elasticsearch 5, use_dis_max is true by default. So maybe it's not a good idea to specify it.
I found a potential bug. To reproduce: 1. Use advanced search 2. Go to the detail page of a result 3. Click on "Return to results" (in the left column) 4. The advsearch URL param is gone Also, I think it should be implemented for OPAC as well. Or at least show a message in the configuration page saying that this feature only works for intranet.
(In reply to Julian Maurice from comment #103) > I found a potential bug. To reproduce: > 1. Use advanced search > 2. Go to the detail page of a result > 3. Click on "Return to results" (in the left column) > 4. The advsearch URL param is gone > This bug already happens even without this patch. Also for example if you have specified multiple keywords in the search only one will be left after clicking the return button.
(In reply to Joonas Kylmälä from comment #104) > This bug already happens even without this patch. Also for example if you > have specified multiple keywords in the search only one will be left after > clicking the return button. Is there already a bug report open for this problem ?
Created attachment 80278 [details] [review] Bug 18316: remove use_dis_max It is true by default on ES 5 and has been removed on ES 6
(In reply to Alex Arnaud from comment #106) > Created attachment 80278 [details] [review] [review] > Bug 18316: remove use_dis_max There should be a (follow-up) in the title. Otherwise looks good and will sign-off after the change.
(In reply to Julian Maurice from comment #105) > (In reply to Joonas Kylmälä from comment #104) > > This bug already happens even without this patch. Also for example if you > > have specified multiple keywords in the search only one will be left after > > clicking the return button. > > Is there already a bug report open for this problem ? yes: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20114
(In reply to Joonas Kylmälä from comment #108) > > Is there already a bug report open for this problem ? > > yes: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20114 Thanks
Created attachment 80335 [details] [review] Bug 18316: (follow-up) remove use_dis_max It is true by default on ES 5 and has been removed on ES 6
Created attachment 81004 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81005 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on all fields plus those with the coresponding weights. Else, search is done on the entire record with no weighting. The advanced search page is unaffected by these changes Test plan (having Koha working with Elasticsearch): - apply this patch - have some weights defined for various fields - try searches from the search bar and from the advanced search page - confirm weighting affects the relevancy (in expected ways) e.g. 1. search for 'a' from advanced search, note results 2. give 'title' a weight 3. search for 'a' using the simple search bar 4. results with 'a' in the title should now be more relevant - confirm search results on advanced search page are unaffected Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81006 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81007 [details] [review] Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81008 [details] [review] Bug 18316: Change search field weight field to decimal Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81009 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm https://bugs.koha-community.org/show_bug.cgi?id=18316 Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81010 [details] [review] Bug 18316: Fix field weight validation/deletion bug Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81011 [details] [review] Bug 18316: (follow-up) Fix tests and update statement Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 81012 [details] [review] Bug 18316: (follow-up) remove use_dis_max It is true by default on ES 5 and has been removed on ES 6 Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
I took the liberty of fixing the md5sum of the schema file while signing off.
Created attachment 81507 [details] [review] Bug 18316: Ability to weight search fields Weight can be defined between 1 and 99. only search fields mapped with biblios can be weighted Test plan: - apply this patch, - update schema file (perl misc/devel/update_dbix_class_files.pl), - go to Administration > Search engine configuration, - in search fields tab, define weights for some fields and save, - check weights has been saved, - reset some weights (empty) and save, - check weights has been reset Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81508 [details] [review] Bug 18316: Add weighting/relevancy options to ES query on simple search The idea is the following: if some search field(s) are weighted in search engine config page, Koha will query ES on all fields plus those with the coresponding weights. Else, search is done on the entire record with no weighting. The advanced search page is unaffected by these changes Test plan (having Koha working with Elasticsearch): - apply this patch - have some weights defined for various fields - try searches from the search bar and from the advanced search page - confirm weighting affects the relevancy (in expected ways) e.g. 1. search for 'a' from advanced search, note results 2. give 'title' a weight 3. search for 'a' using the simple search bar 4. results with 'a' in the title should now be more relevant - confirm search results on advanced search page are unaffected Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81509 [details] [review] Bug 18316: String changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81510 [details] [review] Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81511 [details] [review] Bug 18316: Change search field weight field to decimal Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81512 [details] [review] Bug 18316: Update Koha/Schema/Result/SearchField.pm https://bugs.koha-community.org/show_bug.cgi?id=18316 Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81513 [details] [review] Bug 18316: Fix field weight validation/deletion bug Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81514 [details] [review] Bug 18316: (follow-up) Fix tests and update statement Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81515 [details] [review] Bug 18316: (follow-up) remove use_dis_max It is true by default on ES 5 and has been removed on ES 6 Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81516 [details] [review] Bug 18316: (QA follow-up) Add final missing filter Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Passing QA! I can't find any regressions and the code looks reasonable.. well done for sticking with it everyone and thank you for the perseverance. One minor note.. the bug claims it's sponsored but I can't see by whom... if there is a sponsor can we have it added to the commits with the Sponsored-by trailer?
Awesome work all! Pushed to master for 18.11
Created attachment 82050 [details] [review] Bug 18316: (follow-up) Don't let 'fields' be undef and adjust tests
(In reply to Nick Clemens from comment #133) > Created attachment 82050 [details] [review] [review] > Bug 18316: (follow-up) Don't let 'fields' be undef and adjust tests Followup pushed to master
Basing my current work on bug 20589 on this I think I discovered a regression. In "Bug 18316: Update Koha/Schema/Result/SearchField.pm" the many-to-many relationship (which has previously been manually added) was removed. This causes a crash when resetting or adding new mappings. I can supply a patch for this and will submit in this issue.
Hmm, no second thought this seems to have been done in Koha-master. So perhaps release-manager took care of this. I will fix this in my local branch then and RM will have to fix this manually again where regenerating the schema files for that in case it get merged.
Enhancement, will not be backported to 18.05.x series.