Bug 18316 - Add weighting/relevancy options to ElasticSearch
Summary: Add weighting/relevancy options to ElasticSearch
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 20388 20589 20602 20607
  Show dependency treegraph
 
Reported: 2017-03-22 15:36 UTC by Nick Clemens
Modified: 2018-06-01 09:44 UTC (History)
9 users (show)

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


Attachments
Bug 18316 - Add weighting/relevancy options to ElasticSearch (4.04 KB, patch)
2017-03-22 15:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ElasticSearch (4.02 KB, patch)
2018-03-21 13:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ElasticSearch (4.08 KB, patch)
2018-03-21 13:40 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Ability to weight search fields (11.81 KB, patch)
2018-03-30 15:20 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ES query on simple search (6.78 KB, patch)
2018-03-30 15:20 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Ability to weight search fields (11.95 KB, patch)
2018-04-04 10:01 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ES query on simple search (6.93 KB, patch)
2018-04-04 10:01 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316 - String changes (1.51 KB, patch)
2018-04-12 16:13 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.01 KB, patch)
2018-04-12 16:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (6.98 KB, patch)
2018-04-12 16:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: String changes (1.56 KB, patch)
2018-04-12 16:52 UTC, Nick Clemens
Details | Diff | Splinter 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 (9.67 KB, patch)
2018-04-23 12:22 UTC, Alex Arnaud
Details | Diff | Splinter 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 (9.63 KB, patch)
2018-04-23 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.07 KB, patch)
2018-04-23 16:43 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.04 KB, patch)
2018-04-23 16:43 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316: String changes (1.71 KB, patch)
2018-04-23 16:43 UTC, Séverine Queune
Details | Diff | Splinter 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 (9.77 KB, patch)
2018-04-23 16:44 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.13 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.09 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter 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 (9.82 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Change search field weight field to decimal and add column declaration to DBIx results source (5.63 KB, patch)
2018-05-21 12:26 UTC, David Gustafsson
Details | Diff | Splinter Review
Update Koha/Schema/Result/SearchField.pm (1.86 KB, patch)
2018-05-23 13:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:00 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.12 KB, patch)
2018-05-24 09:07 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter 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 (9.85 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.05 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.17 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.12 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter 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 (9.85 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.05 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.15 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.12 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter 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 (9.85 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.04 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.45 KB, patch)
2018-05-24 13:23 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.07 KB, patch)
2018-05-31 12:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Fix field weight validation/deletion bug (2.91 KB, patch)
2018-06-01 09:43 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-03-22 15:36:16 UTC

    
Comment 1 Nick Clemens 2017-03-22 15:39:16 UTC Comment hidden (obsolete)
Comment 2 Alex Arnaud 2018-03-21 13:38:31 UTC Comment hidden (obsolete)
Comment 3 Alex Arnaud 2018-03-21 13:40:41 UTC Comment hidden (obsolete)
Comment 4 Gaetan Boisson 2018-03-28 12:41:14 UTC
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.
Comment 5 Nick Clemens 2018-03-28 13:07:17 UTC
This sounds like a reasonable approach, looking forward to seeing it :-)
Comment 6 Alex Arnaud 2018-03-30 15:20:40 UTC Comment hidden (obsolete)
Comment 7 Alex Arnaud 2018-03-30 15:20:49 UTC Comment hidden (obsolete)
Comment 8 Séverine Queune 2018-04-04 10:01:25 UTC Comment hidden (obsolete)
Comment 9 Séverine Queune 2018-04-04 10:01:28 UTC Comment hidden (obsolete)
Comment 10 Séverine Queune 2018-04-04 14:33:28 UTC
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.
Comment 11 Alex Arnaud 2018-04-04 15:11:45 UTC
(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?
Comment 12 Nicolas Legrand 2018-04-11 11:21:57 UTC
(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.
Comment 13 Nick Clemens 2018-04-12 11:28:30 UTC
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)
Comment 14 Alex Arnaud 2018-04-12 15:57:17 UTC
(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.
Comment 15 Alex Arnaud 2018-04-12 16:13:59 UTC
Created attachment 74112 [details] [review]
Bug 18316 - String changes
Comment 16 Nick Clemens 2018-04-12 16:52:14 UTC Comment hidden (obsolete)
Comment 17 Nick Clemens 2018-04-12 16:52:18 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2018-04-12 16:52:21 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2018-04-18 20:33:43 UTC
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.
Comment 20 Alex Arnaud 2018-04-19 12:04:18 UTC
(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?
Comment 21 Alex Arnaud 2018-04-23 12:22:23 UTC Comment hidden (obsolete)
Comment 22 Alex Arnaud 2018-04-23 12:50:45 UTC Comment hidden (obsolete)
Comment 23 Séverine Queune 2018-04-23 16:43:51 UTC Comment hidden (obsolete)
Comment 24 Séverine Queune 2018-04-23 16:43:55 UTC Comment hidden (obsolete)
Comment 25 Séverine Queune 2018-04-23 16:43:59 UTC Comment hidden (obsolete)
Comment 26 Séverine Queune 2018-04-23 16:44:02 UTC Comment hidden (obsolete)
Comment 27 Séverine Queune 2018-04-23 16:46:03 UTC
Tested with chinese, persian and latin (english/french) languages.
Relevance is the same as before (with the first patch) so good for me !
Comment 28 David Gustafsson 2018-04-24 19:23:54 UTC
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.
Comment 29 Katrin Fischer 2018-04-27 06:32:29 UTC
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.
Comment 30 Alex Arnaud 2018-04-27 13:08:07 UTC
(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.
Comment 31 Alex Arnaud 2018-05-04 12:50:19 UTC
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>
Comment 32 Alex Arnaud 2018-05-04 12:50:32 UTC
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>
Comment 33 Alex Arnaud 2018-05-04 12:50:45 UTC
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>
Comment 34 Alex Arnaud 2018-05-04 12:50:57 UTC
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>
Comment 35 David Gustafsson 2018-05-09 09:22:54 UTC
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.
Comment 36 David Gustafsson 2018-05-09 09:41:16 UTC
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.
Comment 37 David Gustafsson 2018-05-21 12:26:29 UTC
Created attachment 75458 [details] [review]
Change search field weight field to decimal and add column declaration to DBIx results source
Comment 38 Alex Arnaud 2018-05-23 09:16:35 UTC
(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
Comment 39 Katrin Fischer 2018-05-23 10:33:18 UTC
(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)
Comment 40 David Gustafsson 2018-05-23 12:42:36 UTC
(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?
Comment 41 Nick Clemens 2018-05-23 12:45:23 UTC
(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
Comment 42 Alex Arnaud 2018-05-23 13:52:57 UTC
(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?
Comment 43 David Gustafsson 2018-05-23 13:59:25 UTC
Created attachment 75490 [details] [review]
Update Koha/Schema/Result/SearchField.pm
Comment 44 David Gustafsson 2018-05-23 14:00:33 UTC
(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.
Comment 45 David Gustafsson 2018-05-23 14:04:00 UTC
(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.
Comment 46 Katrin Fischer 2018-05-23 14:33:29 UTC
(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.
Comment 47 Alex Arnaud 2018-05-24 07:46:07 UTC
(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.
Comment 48 David Gustafsson 2018-05-24 09:00:14 UTC
Created attachment 75519 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 49 David Gustafsson 2018-05-24 09:04:16 UTC
Created attachment 75520 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 50 David Gustafsson 2018-05-24 09:07:56 UTC
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>
Comment 51 David Gustafsson 2018-05-24 09:08:11 UTC
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>
Comment 52 David Gustafsson 2018-05-24 09:08:38 UTC
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>
Comment 53 David Gustafsson 2018-05-24 09:08:44 UTC
Created attachment 75524 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 54 David Gustafsson 2018-05-24 09:08:50 UTC
Created attachment 75525 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 55 David Gustafsson 2018-05-24 09:14:24 UTC
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>
Comment 56 David Gustafsson 2018-05-24 09:14:28 UTC
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>
Comment 57 David Gustafsson 2018-05-24 09:14:32 UTC
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>
Comment 58 David Gustafsson 2018-05-24 09:14:35 UTC
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>
Comment 59 David Gustafsson 2018-05-24 09:14:39 UTC
Created attachment 75530 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 60 David Gustafsson 2018-05-24 09:14:43 UTC
Created attachment 75531 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 61 David Gustafsson 2018-05-24 09:18:20 UTC
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.
Comment 62 Séverine Queune 2018-05-24 12:45:24 UTC
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 :/
Comment 63 David Gustafsson 2018-05-24 13:14:23 UTC
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.
Comment 64 David Gustafsson 2018-05-24 13:22:42 UTC
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>
Comment 65 David Gustafsson 2018-05-24 13:22:46 UTC
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>
Comment 66 David Gustafsson 2018-05-24 13:22:50 UTC
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>
Comment 67 David Gustafsson 2018-05-24 13:22:54 UTC
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>
Comment 68 David Gustafsson 2018-05-24 13:22:58 UTC
Created attachment 75540 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 69 David Gustafsson 2018-05-24 13:23:02 UTC
Created attachment 75541 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 70 David Gustafsson 2018-05-24 13:24:08 UTC
Now patches should apply to latest master.
Comment 71 David Gustafsson 2018-05-25 17:15:12 UTC
Forgot to change status.
Comment 72 Séverine Queune 2018-05-30 08:54:01 UTC
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
Comment 73 Jonathan Druart 2018-05-30 14:40:00 UTC
(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
Comment 74 David Gustafsson 2018-05-30 14:51:36 UTC
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.
Comment 75 David Gustafsson 2018-05-31 12:11:53 UTC
Created attachment 75699 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 76 David Gustafsson 2018-05-31 12:14:03 UTC
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.
Comment 77 Alex Arnaud 2018-06-01 08:38:09 UTC
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.
Comment 78 David Gustafsson 2018-06-01 09:43:34 UTC
Created attachment 75730 [details] [review]
Bug 18316: Fix field weight validation/deletion bug
Comment 79 David Gustafsson 2018-06-01 09:44:52 UTC
Ok! This should now be fixed.