Bug 12430

Summary: Relevance ranking should also be used without QueryWeightFields system preference
Product: Koha Reporter: David Cook <dcook>
Component: Searching - ZebraAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: major    
Priority: P5 - low CC: andrewfh, bernard.scaife, caroline.cyr-la-rose, david, fridolin.somers, jhannert, jonathan.druart, katrin.fischer, koha, martin.renvoize, mirko, ssammons, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16581
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.07,19.11.13
Attachments: Bug 12430 - Truncation disables relevance ranking when not using QueryParser
Bug 12430 - Truncation disables relevance ranking when not using QueryParser
Bug 12430 - Truncation disables relevance ranking when not using QueryParser
Bug 12430 - Truncation disables relevance ranking when not using QueryParser
Bug 12430: Force relevance ranking when not using QueryParser
First Draft buildQuery unit test
Unit tests for buildQuery
Bug 12430: Force relevance ranking when not using QueryParser
Unit tests for buildQuery
Bug 12430: Use releance ranking without QueryWeightFields
Bug 12430: Add unit tests for C4::Search::buildQuery
Bug 12430: Use releance ranking without QueryWeightFields
Bug 12430: Add unit tests for C4::Search::buildQuery
Bug 12430: Add unit tests for C4::Search::buildQuery
Bug 12430: Fix selenium/regressions.t failure
Bug 12430: Fix Search.t failure

Description David Cook 2014-06-17 02:50:06 UTC
Scenario:

1) NOT using QueryParser
2) Using truncation (either by explicitly using * in the query or when 'QueryAutoTruncate' is set to 'automatically')

Result:

1) Search results ARE ordered by biblionumber ascending and NOT by relevance

--

This appears not to be an issue when using QueryParser as it seems to parse queries much better than the traditional...method.

However, quite a major issue for those not using QueryParser, as everyone likes relevant searches.
Comment 1 David Cook 2014-06-17 04:46:17 UTC
So the QueryParser actually acts a bit oddly as well.

In some cases, it will add the relevance attribute to a query with a couple weighted fields, as defined in this section of queryparser.yaml:

relevance_bumps:
  keyword:
    titleext:
      enabled: 1
      bib1_mapping:
        biblioserver: 34
    titlekw:
      enabled: 1
      bib1_mapping:
        biblioserver: 20

If you type in "test" into the OPAC as a "Library catalog" search with QueryAutoTruncate (or as test*), QueryParser will generate something like this:

@or @or @attr 1=1016 @attr 5=1 @attr 4=6 "test" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "test"

Let me re-organize that a bit so it looks a bit more readable by humans...

@attr 1=1016 @attr 5=1 @attr 4=6 "test" 
@or
@attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "test"
@or
@attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "test"

Basically, it says it's doing a keyword (@attr 1=1016) wordlist (@attr 4=6) search for a right truncated (@attr 5=1) "test" in 3 branches of this query. 

In two branches, it's looking for relevance (@attr 2=102) with weighted fields (@attr 9=20) and (@attr 9=34) where the former is worth fewer points than the latter in the ranking.

Source: http://www.indexdata.com/zebra/doc/administration-ranking.html

--

If you do a "Title" search for "test", you'll get the following:

@attr 1=4 @attr 5=1 @attr 4=6 "test"

(@attr 5=1 means 'right truncate', @attr 4=6 means 'wordlist', and @attr 1=4 means 'title')

Notice that we no longer have relevance ranking (via @attr 2=102) and no weighted fields (@attr 9=VALUE)

--

So...it seems to me that relevance only works with QueryParser for keywords...and even then in an unexpected way...
Comment 2 David Cook 2014-06-17 06:18:13 UTC
There's a bit of odd code in  Koha\QueryParser\Driver\PQF\query_plan\node.pm, which is where a lot of the action happens in QP...

When doing a "Library Catalog" search, it tries to use the following code to create a relevance query branch:

$fieldobj = $self->plan->QueryParser->bib1_mapping_by_name('field', $relbump->{'classname'}, $relbump->{'field'}, $server);

This means $fieldobj, which should contain our CCL2RPN type mapping, will always be null because $relbump->{'classname'} and $relbump->{'field'} don't exist.

This is why the 2nd line in the following code doesn't have an @attr 1=1016 in it. 

@attr 1=1016 @attr 5=1 @attr 4=6 "test" 
@or
@attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "test"

But we don't notice the error in the OPAC, as I think Zebra/Yaz/Zoom/whatever silently treats the second branch as having @attr 1=1016 as a bare search will usually be treated as a keyword (ie @attr 1=1016). 

--

Here is another odd bit of code, which is responsible for QueryParser's current "support" for relevance.

my $relbumps = $self->plan->QueryParser->bib1_mapping_by_name('relevance_bump', $self->classname, '', $server);

This line will return ALL the relevance_bump mappings under the keyword class...which are 'titleext' and 'titlekw'.

I think these may have been example mappings...as it doesn't really make sense to have them under a keyword class search. It should be a "keyword" class search with a "keyword" field rather than a "titleext" field, in my opinion.
Comment 3 David Cook 2014-06-17 06:23:10 UTC
In any case, you can add relevance mappings to QueryParser by adding additional config in queryparser.yaml.

For instance:
  title:
    title:
      enabled: 1
      bib1_mapping:
        biblioserver: 30

This will add relevance ranking to a title search with a field weight of 30.

However, it's worth noting that QueryParser seems to often fall back to non-QueryParser mode for more complicated indexes like "ti,phr", so...I wouldn't go out and make a lot of QP mappings just yet...
Comment 4 David Cook 2014-06-17 07:17:36 UTC
Created attachment 28880 [details] [review]
Bug 12430 - Truncation disables relevance ranking when not using QueryParser

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

1) Turn on "QueryWeightFields" and "QueryAutoTruncate"
2) Turn off "TryQueryParser"
3) Do a keyword search in the OPAC
4) Note that the results are ordered by biblionumber ascending

5) Apply patch

6) Do the same keyword search in the OPAC
7) Note that the results are never ordered differently
(presumably in relevance order :P)

If you're a stickler, throw in some warns so that you can see
what CCL query is being sent to Zebra.
Comment 5 David Cook 2014-06-17 23:35:36 UTC
Changing the importance of this one as I suppose its severity is debateable...
Comment 6 David Cook 2014-06-18 01:25:21 UTC
It appears that this patch could break the "Publication Date (yyyy)" search, when using "yr,st-numeric" (which might be the case after bug 9368).

If we were to use "yr,st-year" instead, you might get results for a "2000-" query but they would only be for "2000". If you specified a full range "2000-2012", it would break.

It seems that relevance can't be run with date ranges (which is usually fine, as we use date ranges as limits which are not affected by relevance ranking).
Comment 7 David Cook 2014-06-18 05:31:34 UTC
Putting this on hold for the moment while I try to get some refactoring done on "buildQuery".

We shouldn't be adding relevance searching to date ranges (it will cause errors that produce no results or results only matching the first date in the range) or item-level indexes (as the relevance score will be artificially inflated by bib records with higher numbers of items).
Comment 8 David Cook 2016-05-16 00:07:18 UTC
(In reply to David Cook from comment #6)
> It appears that this patch could break the "Publication Date (yyyy)" search,
> when using "yr,st-numeric" (which might be the case after bug 9368).
> 
> If we were to use "yr,st-year" instead, you might get results for a "2000-"
> query but they would only be for "2000". If you specified a full range
> "2000-2012", it would break.
> 
> It seems that relevance can't be run with date ranges (which is usually
> fine, as we use date ranges as limits which are not affected by relevance
> ranking).

FYI, the issue with st-numeric was fixed in Zebra 2.0.62

http://www.indexdata.com/zebra/doc/NEWS
Comment 9 David Cook 2016-05-16 00:07:58 UTC
Also, I'm probably never going to work on this again.

I don't have the time to fix all the things that are wrong with C4::Search.
Comment 10 David Cook 2016-05-16 00:11:38 UTC
Honestly, there's probably no point trying to refactor the C4::Search code for Zebra anymore. It would probably be better to scrap it and start over rather than try to make it work the way it does now.

Time would probably be better spent finishing the QueryParser, or converting Koha to use actual CCL query syntax.
Comment 11 Mirko Tietgen 2016-06-16 14:38:11 UTC
(In reply to David Cook from comment #8)

> FYI, the issue with st-numeric was fixed in Zebra 2.0.62
> 
> http://www.indexdata.com/zebra/doc/NEWS

Hi David, are there other reasons not to use this patch?
Comment 12 Shane Sammons 2018-04-09 18:05:25 UTC
I just wanted to note that search result being relevant is an issue. I just migrated from 3.20.00 to 17.11(Debian Packages on Ubuntu 16.04LTS) and I had to change Trunction to require *, otherwise expected top results were mixed in the results.

I don't care if it is a fix switching to other "stack", a patch to this issue, or whatever, but I wanted to comment that something should be done.

Thankfully you are all a great community and super helpful so IRC chat helped me learn about this bug and we turned it from auto to require * to get far better search results.
Comment 13 Katrin Fischer 2018-04-15 09:32:26 UTC
I am upping severity here since a lot of people use QueryAutoTruncate unaware of it breaking Relevance and falsely assuming Koha can't do better. If we can't fix it, we should add a warning to the system preference description of QueryAutoTruncate.

Old patch doesn't apply any longer.
Comment 14 David Cook 2018-04-17 05:25:24 UTC
(In reply to Katrin Fischer from comment #13)
> I am upping severity here since a lot of people use QueryAutoTruncate
> unaware of it breaking Relevance and falsely assuming Koha can't do better.
> If we can't fix it, we should add a warning to the system preference
> description of QueryAutoTruncate.
> 
> Old patch doesn't apply any longer.

The old patch was problematic anyway as I note in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12430#c6. It looks like it would be OK with Zebra 2.0.62 but Debian Jessie and Debian Stretch both use Zebra 2.0.59: https://packages.debian.org/jessie/text/idzebra-2.0 and https://packages.debian.org/stretch/idzebra-2.0.

As far as I can tell, with older versions of Zebra, there simply is no real practical all-encompassing solution.
Comment 15 Martin Renvoize 2019-12-10 17:05:54 UTC
Created attachment 96167 [details] [review]
Bug 12430 - Truncation disables relevance ranking when not using QueryParser

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

1) Turn on "QueryWeightFields" and "QueryAutoTruncate"
2) Turn off "TryQueryParser"
3) Do a keyword search in the OPAC
4) Note that the results are ordered by biblionumber ascending

5) Apply patch

6) Do the same keyword search in the OPAC
7) Note that the results are never ordered differently
(presumably in relevance order :P)

If you're a stickler, throw in some warns so that you can see
what CCL query is being sent to Zebra.
Comment 16 Martin Renvoize 2019-12-10 17:06:47 UTC
I've attempted a rebase of the patch.. it's not an area I'm all that familiar with so testing is required.
Comment 17 Jonathan Druart 2019-12-12 14:30:00 UTC
(In reply to Martin Renvoize from comment #16)
> I've attempted a rebase of the patch.. it's not an area I'm all that
> familiar with so testing is required.

Reading previous comments it seems that this patch will introduce regressions.
Comment 18 Victor Grousset/tuxayo 2020-01-30 22:07:58 UTC
Created attachment 98188 [details] [review]
Bug 12430 - Truncation disables relevance ranking when not using QueryParser

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

1) Turn on "QueryWeightFields" and set "QueryAutoTruncate" to auto
2) Turn off "UseQueryParser"
3) Do a keyword search in the OPAC
4) Note that the results are ordered by biblionumber ascending

5) Apply patch

6) Do the same keyword search in the OPAC
7) Note that the results are never ordered differently
(presumably in relevance order :P)

If you're a stickler, throw in some warns so that you can see
what CCL query is being sent to Zebra.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Victor Grousset/tuxayo 2020-01-30 22:13:42 UTC
> I've attempted a rebase of the patch.. 
> it's not an area I'm all that familiar with so testing is required.

- test plan passed
- signed off
- fixed test plan: TryQueryParser => UseQueryParser
- clarified test plan: "enable QueryAutoTruncate" =>  "set QueryAutoTruncate to auto"

As per the uncertainty about regressions, should the ticket status be "In discussion" ?
Comment 20 Jonathan Druart 2020-01-31 09:56:28 UTC
(In reply to Victor Grousset/tuxayo from comment #19)
> > I've attempted a rebase of the patch.. 
> > it's not an area I'm all that familiar with so testing is required.
> 
> - test plan passed
> - signed off
> - fixed test plan: TryQueryParser => UseQueryParser
> - clarified test plan: "enable QueryAutoTruncate" =>  "set QueryAutoTruncate
> to auto"
> 
> As per the uncertainty about regressions, should the ticket status be "In
> discussion" ?

Did you try the problematic cases?
Comment 21 Martin Renvoize 2020-02-04 14:12:51 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Martin Renvoize from comment #16)
> > I've attempted a rebase of the patch.. it's not an area I'm all that
> > familiar with so testing is required.
> 
> Reading previous comments it seems that this patch will introduce
> regressions.

I think there are regressions for specific versions of zebra.. but I also felt that relevance issues outweighted the regressions introduced.
Comment 22 Martin Renvoize 2020-02-04 14:14:58 UTC
De-escalating this one.. it's been around for 6 years and hasn't really recieved the attention a critical should.. I personally feel at the time it was critical and it's still a nice to fix.. but as we're moving toward elastic search and possible deprecating QueryParser entirely I'm not sure how to progress this bug.
Comment 23 Katrin Fischer 2020-02-04 18:51:13 UTC
(In reply to Martin Renvoize from comment #22)
> De-escalating this one.. it's been around for 6 years and hasn't really
> recieved the attention a critical should.. I personally feel at the time it
> was critical and it's still a nice to fix.. but as we're moving toward
> elastic search and possible deprecating QueryParser entirely I'm not sure
> how to progress this bug.

I'd argue it's still a very bad bug and Elastic not ready yet for wide use yet (thinking of libraries without IT experts and support especially). As it breaks things when NOT using QueryParser, there is no link to that. It hurts a lot of our libraries and is very hard to figure out - I think we shoudl really fix it. The sole reason it was not fixed yet is probably people are scared of Search.pm.
Comment 24 David Cook 2020-02-05 00:33:07 UTC
(In reply to Katrin Fischer from comment #23)
> I'd argue it's still a very bad bug and Elastic not ready yet for wide use
> yet (thinking of libraries without IT experts and support especially). As it
> breaks things when NOT using QueryParser, there is no link to that. It hurts
> a lot of our libraries and is very hard to figure out - I think we shoudl
> really fix it. The sole reason it was not fixed yet is probably people are
> scared of Search.pm.

I'm not scared but there's no time/money for me to work on fixing it :/. 

In my opinion, Search.pm is really really bad, and fixing it would be a very time-consuming exercise. (I suppose part of the time-consuming part would be finding a lack of people wiling to test/QA because of fear of Search.pm though that's true...)
Comment 25 Jonathan Druart 2020-02-05 08:19:52 UTC
(In reply to David Cook from comment #24)
> (In reply to Katrin Fischer from comment #23)
> > I'd argue it's still a very bad bug and Elastic not ready yet for wide use
> > yet (thinking of libraries without IT experts and support especially). As it
> > breaks things when NOT using QueryParser, there is no link to that. It hurts
> > a lot of our libraries and is very hard to figure out - I think we shoudl
> > really fix it. The sole reason it was not fixed yet is probably people are
> > scared of Search.pm.
> 
> I'm not scared but there's no time/money for me to work on fixing it :/. 
> 
> In my opinion, Search.pm is really really bad, and fixing it would be a very
> time-consuming exercise. (I suppose part of the time-consuming part would be
> finding a lack of people wiling to test/QA because of fear of Search.pm
> though that's true...)

What about the patch? Was not it suppose to fix the issue?
Comment 26 David Cook 2020-02-06 00:11:49 UTC
(In reply to Jonathan Druart from comment #25)
> 
> What about the patch? Was not it suppose to fix the issue?

Oh yeah... apparently I wrote that patch 6 years ago. I didn't realize I was even the author of it. 

But according to my comment from https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12430#c6, I think my patch would actually break some searches in Koha where older versions of Zebra are used (ie < 2.0.62). I think the koha-common package uses 2.0.59 by default, so probably not a good idea to use this patch.

I think my hopelessness with Search.pm led me to making this comment https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12430#c9 and opening this bug yesterday: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24586

I think the problem with Koha's search is that it looks like it works in many cases, so attempts to fix it will change the familiar behaviour, and people will complain.

But I don't think it's possible to fix it and maintain backwards compatibility with current behaviour. 

The hardest part for me is finding the resources to fix it. Because it mostly works, I don't have a client-led or employer-led mandate to spend my resources fixing it. I wish I could do it out of the goodness of my heart, but my heart has other priorities.
Comment 27 David Cook 2020-02-06 00:18:37 UTC
I have been thinking that maybe we should modularize search more so that it would make it easier to customize behaviour, unit test, and swap different backends in/out, but... I think one of the biggest struggles we face is that Koha doesn't really have a search query language. 

We have something like CCL, but it's not CCL, and our compensating for that fact leads to problems. And without a real query language, we don't have a real query parser, and without a real query parser, it's hard to safely and programmatically alter the queries. 

But... I don't really have a solution in mind either. Sometimes, I hope that we'll just get rid of Zebra, and then we can just focus on Elasticsearch, and maybe change our search query syntax to be safer, easier to parse, and easier to integrate with Elasticsearch. I think it would be a breaking change away from the Zebra CCL-like search syntax we've used... but that would probably be a good thing.
Comment 28 Martin Renvoize 2020-02-07 12:47:55 UTC
(In reply to David Cook from comment #27)
> I have been thinking that maybe we should modularize search more so that it
> would make it easier to customize behaviour, unit test, and swap different
> backends in/out, but... I think one of the biggest struggles we face is that
> Koha doesn't really have a search query language. 
> 
> We have something like CCL, but it's not CCL, and our compensating for that
> fact leads to problems. And without a real query language, we don't have a
> real query parser, and without a real query parser, it's hard to safely and
> programmatically alter the queries. 
> 
> But... I don't really have a solution in mind either. Sometimes, I hope that
> we'll just get rid of Zebra, and then we can just focus on Elasticsearch,
> and maybe change our search query syntax to be safer, easier to parse, and
> easier to integrate with Elasticsearch. I think it would be a breaking
> change away from the Zebra CCL-like search syntax we've used... but that
> would probably be a good thing.

Agreed, but I feel that's for another bug ;)
Comment 29 Martin Renvoize 2020-02-07 12:48:45 UTC
Regarding this bug, I think we should use the patch here and update our minimum required zebra version (and document how to get said zebra version via either the indexdata repo's or pulling a copy into the koha repo)
Comment 30 Katrin Fischer 2020-02-09 12:03:16 UTC
(In reply to Martin Renvoize from comment #29)
> Regarding this bug, I think we should use the patch here and update our
> minimum required zebra version (and document how to get said zebra version
> via either the indexdata repo's or pulling a copy into the koha repo)

+1
Comment 31 Katrin Fischer 2020-02-09 12:18:54 UTC
It appears there are other known and sever issues with .59 - so that could lead as another motivation to move us to a newer version by default: bug 16581
Comment 32 Katrin Fischer 2020-02-09 12:31:00 UTC
Search example, for testing with our test data:
- Who
The Doctor Who books move down the list without this patch, but are on top with it.
Comment 33 Katrin Fischer 2020-02-09 12:38:02 UTC
(In reply to David Cook from comment #6)
> It appears that this patch could break the "Publication Date (yyyy)" search,
> when using "yr,st-numeric" (which might be the case after bug 9368).
> 
> If we were to use "yr,st-year" instead, you might get results for a "2000-"
> query but they would only be for "2000". If you specified a full range
> "2000-2012", it would break.
> 
> It seems that relevance can't be run with date ranges (which is usually
> fine, as we use date ranges as limits which are not affected by relevance
> ranking).

I can't find any issues with my kohadevbox using Zebra 2.0.59.

Tested on our sample data with the following date range searches from advanced search, receiving the correct results:

1941
-1941
1900-1941
2000
2000-

Adding my sign-off to this, as all tests so far are positive.
Comment 34 Katrin Fischer 2020-02-09 12:38:30 UTC
Created attachment 98619 [details] [review]
Bug 12430 - Truncation disables relevance ranking when not using QueryParser

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

1) Turn on "QueryWeightFields" and set "QueryAutoTruncate" to auto
2) Turn off "UseQueryParser"
3) Do a keyword search in the OPAC
4) Note that the results are ordered by biblionumber ascending

5) Apply patch

6) Do the same keyword search in the OPAC
7) Note that the results are never ordered differently
(presumably in relevance order :P)

If you're a stickler, throw in some warns so that you can see
what CCL query is being sent to Zebra.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 David Cook 2020-02-10 01:19:21 UTC
(In reply to Martin Renvoize from comment #29)
> Regarding this bug, I think we should use the patch here and update our
> minimum required zebra version (and document how to get said zebra version
> via either the indexdata repo's or pulling a copy into the koha repo)

+1
Comment 36 Jonathan Druart 2020-02-17 11:00:34 UTC
Can you put commit title and bug report title in sync please?

I guess nobody will have time to provide tests to cover this change?
Comment 37 David Cook 2020-02-17 23:12:41 UTC
(In reply to Jonathan Druart from comment #36)
> Can you put commit title and bug report title in sync please?
> 
> I guess nobody will have time to provide tests to cover this change?

Well when you put it that way... hehe.

I'll put it on my TODO list for tonight. I have a few patches I want to do tonight, so I'll try to get them all done at once.
Comment 38 David Cook 2020-02-18 02:36:43 UTC
(In reply to Jonathan Druart from comment #36)
> Can you put commit title and bug report title in sync please?
> 
Actually, I'm not 100% sure what you mean by this, but I'm going to
Comment 39 David Cook 2020-02-18 02:38:28 UTC
Created attachment 99141 [details] [review]
Bug 12430: Force relevance ranking when not using QueryParser

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

1) Turn on "QueryWeightFields" and set "QueryAutoTruncate" to auto
2) Turn off "UseQueryParser"
3) Do a keyword search in the OPAC
4) Note that the results are ordered by biblionumber ascending

5) Apply patch

6) Do the same keyword search in the OPAC
7) Note that the results are never ordered differently
(presumably in relevance order :P)

If you're a stickler, throw in some warns so that you can see
what CCL query is being sent to Zebra.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 David Cook 2020-02-18 02:39:27 UTC
(In reply to Jonathan Druart from comment #36)
> Can you put commit title and bug report title in sync please?
 
Ok hopefully this is more in line with what you were thinking.

> I guess nobody will have time to provide tests to cover this change?

I'm going to try to do this now in the next 20 minutes...
Comment 41 David Cook 2020-02-18 03:03:18 UTC
Created attachment 99142 [details] [review]
First Draft buildQuery unit test
Comment 42 David Cook 2020-02-18 03:06:45 UTC
(In reply to David Cook from comment #41)
> Created attachment 99142 [details] [review] [review]
> First Draft buildQuery unit test

This isn't complete yet, but it's a first pass before I have to run out of the office for a few hours.

I need to add more permutations to the tests:

1. like a query with no * or QueryAutoTruncate to see if QueryWeightFields still works
2. like a more complex CCL/CCLish query
3. Turn on UseQueryParser (even if I think we should soon remove QueryParser from Koha)
4. Add limits to the unit tests

Note that buildQuery spits out a lot of warnings, but I don't think they're relevant for this test. Rather, I think they're just indicative that buildQuery isn't very well written. Of course, these unit tests could be the basis for improving it. Thanks, Jonathan, for poking me on this one.
Comment 43 David Cook 2020-02-18 07:18:24 UTC
Whoa... added unit tests when UseQueryParser is enabled and it's giving unpredictable results. 

Half the time, the expected result is correct and half the time it is wrong. It looks like it's swapping the order of the operands around. Bizarre.
#          got: '@or @or @attr 1=1016 @attr 5=1 @attr 4=6 "test" @attr 2=102 @attr 9=34 @attr 5=1 @attr 4=6 "test" @attr 2=102 @attr 9=20 @attr 5=1 @attr 4=6 "test"'
#     expected: '@or @or @attr 1=1016 @attr 5=1 @attr 4=6 "test" @attr 2=102 @attr 9=20 @attr 5=1 @attr 4=6 "test" @attr 2=102 @attr 9=34 @attr 5=1 @attr 4=6 "test"'

I don't know what to do about that... it'll unpredictably bust the tests on master, but it is arguably a problem with the QueryParser or at least a problem of evaluating the query based on string matching instead of parsing the query and analyzing it logically...

I think I'll leave out that test for now... especiallys ince we are planning to remove the QueryParser...
Comment 44 David Cook 2020-02-18 08:34:38 UTC
Created attachment 99158 [details] [review]
Unit tests for buildQuery

This patch adds unit tets for buildQuery, especially focusing on
the use of the (rk=()) wrapper.

Note that the Net::Z3950::ZOOM library is used to test the queries
for CCL validity on top of the string matching used in the
got/expected comparisons.
Comment 45 David Cook 2020-02-18 08:42:36 UTC
Probably spent too long on the unit tests, but they should be useful. They test got vs expected values, plus they also do a CCL2RPN test to make sure it's actually valid CCL. 

I think the tests could probably be much more extensive, but they're a good base and they will hopefully be useful showcasing this change.

Although as I say that it looks like the patch isn't actually working... so I'm going to investigate that now...
Comment 46 Katrin Fischer 2020-02-18 08:53:33 UTC
Did you change the first patch too or only added the tests?
Comment 47 David Cook 2020-02-18 08:55:21 UTC
(In reply to Katrin Fischer from comment #46)
> Did you change the first patch too or only added the tests?

I changed the title on the first patch but no code changes.

The only code changes are on the second patch.

--

And false alarm about the first patch not working now. It just turns out my search query "test" returned 3 records that had equal relevance. I updated biblio 134 and added "Test" to the title and now it's appearing at the top of the search results.

Hurray!
Comment 48 Katrin Fischer 2020-02-24 20:48:16 UTC
We've voted to remove the QueryParser code (https://wiki.koha-community.org/wiki/Development_IRC_meeting_4_March_2020), so I am closing this WONTFIX.
Comment 49 Katrin Fischer 2020-02-24 21:01:51 UTC
Oops, this one got falsely linked to QueryParser - it breaks things WITHOUT IT. Removing dependency.
Comment 50 David Cook 2020-02-25 06:04:00 UTC
(In reply to Katrin Fischer from comment #49)
> Oops, this one got falsely linked to QueryParser - it breaks things WITHOUT
> IT. Removing dependency.

Good catch! I was just going through my emails and went "whoops!"
Comment 51 David Nind 2020-03-01 19:40:41 UTC
Everything works as per the the test plan, but tests fail:

root@ff2bb4ecb08d:koha(bz12430)$ prove t/Search.t
t/Search.t .. 1/6 
    #   Failed test 'Limit is correct'
    #   at t/Search.t line 94.
    #          got: ''
    #     expected: undef

    #   Failed test 'Limit desc is correct'
    #   at t/Search.t line 96.
    #          got: ''
    #     expected: undef

    #   Failed test 'Limit is correct'
    #   at t/Search.t line 121.
    #          got: ''
    #     expected: undef

    #   Failed test 'Limit desc is correct'
    #   at t/Search.t line 123.
    #          got: ''
    #     expected: undef

    #   Failed test 'Limit is correct'
    #   at t/Search.t line 148.
    #          got: ''
    #     expected: undef

    #   Failed test 'Limit desc is correct'
    #   at t/Search.t line 150.
    #          got: ''
    #     expected: undef

    #   Failed test 'Limit is correct'
    #   at t/Search.t line 268.
    #          got: ''
    #     expected: undef

    #   Failed test 'Limit desc is correct'
    #   at t/Search.t line 270.
    #          got: ''
    #     expected: undef
    # Looks like you failed 8 tests of 91.

#   Failed test 'buildQuery basic tests'
#   at t/Search.t line 283.
# Looks like you failed 1 test of 6.
t/Search.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/6 subtests 

Test Summary Report
-------------------
t/Search.t (Wstat: 256 Tests: 6 Failed: 1)
  Failed test:  5
  Non-zero exit status: 1
Files=1, Tests=6,  4 wallclock secs ( 0.03 usr  0.01 sys +  3.42 cusr  0.12 csys =  3.58 CPU)
Result: FAIL
Comment 52 David Cook 2020-03-02 02:31:04 UTC
Interesting. I'll make a note to look at this later.
Comment 53 David Cook 2020-03-02 02:31:11 UTC
Interesting. I'll make a note to look at this later.
Comment 54 Katrin Fischer 2020-05-28 15:30:34 UTC
I'd still love to see this fixed.
Comment 55 David Cook 2020-05-28 23:06:17 UTC
(In reply to Katrin Fischer from comment #54)
> I'd still love to see this fixed.

Me too. It's on my TODO list 😅.
Comment 56 Martin Renvoize 2020-08-21 14:30:55 UTC
Ping :)
Comment 57 David Cook 2020-08-23 23:52:57 UTC
(In reply to Martin Renvoize from comment #56)
> Ping :)

I don't really want to work on this again, but it would be a good thing to have. I'll put it on the list again.
Comment 58 Katrin Fischer 2020-09-13 09:46:11 UTC
Where did we leave off here? The fix working but the tests failing? It would be so great to have this finally fixed.
Comment 59 David Cook 2020-09-13 23:17:54 UTC
(In reply to Katrin Fischer from comment #58)
> Where did we leave off here? The fix working but the tests failing? It would
> be so great to have this finally fixed.

Yeah, I think that's where we left off. 

This one is pretty low priority for me at the moment, so happy for someone else to take it over.
Comment 60 David Cook 2020-12-03 01:04:39 UTC
Actually, I got caught out by this one today, so I think I will have another crack at this one.
Comment 61 David Cook 2020-12-03 03:28:53 UTC
root@kohadevbox:koha(master)$ git checkout bug12430
Switched to branch 'bug12430'
Your branch and 'origin/master' have diverged,
and have 1 and 19114 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)
Comment 62 David Cook 2020-12-03 03:35:11 UTC
(In reply to David Nind from comment #51)
> Everything works as per the the test plan, but tests fail:
> 
> Test Summary Report
> -------------------
> t/Search.t (Wstat: 256 Tests: 6 Failed: 1)
>   Failed test:  5
>   Non-zero exit status: 1
> Files=1, Tests=6,  4 wallclock secs ( 0.03 usr  0.01 sys +  3.42 cusr  0.12
> csys =  3.58 CPU)
> Result: FAIL

It looks like the first patch is good, but my "Unit tests for buildQuery" patch is not.
Comment 63 David Cook 2020-12-03 03:59:12 UTC
Changing the 2nd patch to use t/Search/buildQuery.t instead of t/Search.t because it's so complex it deserves to be in its own file...
Comment 64 David Cook 2020-12-03 05:08:15 UTC
Created attachment 114129 [details] [review]
Bug 12430: Force relevance ranking when not using QueryParser

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

0) Use koha-testing-docker
1) Ensure that "QueryWeightFields" is enabled and "QueryAutoTruncate" is set to auto
2) Go to http://localhost:8080/cgi-bin/koha/opac-search.pl?q=e
3) Note that the results are ordered by biblionumber ascending

4) Apply patch

5) Go to http://localhost:8080/cgi-bin/koha/opac-search.pl?q=e
6) Note that the results are not ordered by biblionumber ascending
(presumably they are in relevance order)

If you edit /etc/koha/sites/kohadev/koha-conf.xml and add "requests"
to the end of "zebra_loglevels", uncomment that option, and carefully
restart Zebra, you should be able to see the following in
/var/log/koha/kohadev/zebra-output.log:

Search biblios OK 394 1 1+0 RPN @attrset Bib-1 @attr 1=1016 @attr 4=6 @attr 5=1 @attr 2=102 e

The 2=102 indicates a relevance search.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 David Cook 2020-12-03 05:08:21 UTC
Created attachment 114130 [details] [review]
Unit tests for buildQuery

This patch adds unit tests for buildQuery, especially focusing on
the use of the (rk=()) wrapper.

Note that the Net::Z3950::ZOOM library is used to test the queries
for CCL validity on top of the string matching used in the
got/expected comparisons.
Comment 66 David Cook 2020-12-03 05:09:15 UTC
To run the unit tests:

root@kohadevbox:koha(bug_12430)$ prove t/Search/buildQuery.t
t/Search/buildQuery.t .. ok
All tests successful.
Files=1, Tests=10,  3 wallclock secs ( 0.03 usr  0.01 sys +  2.87 cusr  0.15 csys =  3.06 CPU)
Result: PASS
Comment 67 David Cook 2020-12-03 05:27:14 UTC
When this is pushed, I'm half tempted to push for a removal of "QueryWeightFields" completely as I reckon it causes more grief than it's worth...
Comment 68 David Cook 2020-12-03 05:48:04 UTC
Very old style commit message name. I might just fix that up now...
Comment 69 David Cook 2020-12-03 05:49:22 UTC
Created attachment 114131 [details] [review]
Bug 12430: Use releance ranking without QueryWeightFields

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

1) Turn on "QueryWeightFields" and set "QueryAutoTruncate" to auto
2) Turn off "UseQueryParser"
3) Do a keyword search in the OPAC
4) Note that the results are ordered by biblionumber ascending

5) Apply patch

6) Do the same keyword search in the OPAC
7) Note that the results are never ordered differently
(presumably in relevance order :P)

If you're a stickler, throw in some warns so that you can see
what CCL query is being sent to Zebra.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 70 David Cook 2020-12-03 05:49:28 UTC
Created attachment 114132 [details] [review]
Bug 12430: Add unit tests for C4::Search::buildQuery

This patch adds unit tests for buildQuery, especially focusing on
the use of the (rk=()) wrapper.

Note that the Net::Z3950::ZOOM library is used to test the queries
for CCL validity on top of the string matching used in the
got/expected comparisons.
Comment 71 David Cook 2020-12-03 05:53:49 UTC
*** Bug 24006 has been marked as a duplicate of this bug. ***
Comment 72 Martin Renvoize 2020-12-03 11:14:54 UTC
Created attachment 114137 [details] [review]
Bug 12430: Use releance ranking without QueryWeightFields

The system preference "QueryWeightFields" is mutually exclusive with
the system preference "QueryAutoTruncate" and the * truncation
modifier, when not using QueryParser.

If you use truncation, relevance won't work anymore. (N.B. Relevance
doesn't work probably when using QueryParser, but for a very different
reason beyond the current scope of this bug.)

This patch adds relevance ranking when using truncation (or basically
when QueryWeightFields is disabled).

_TEST PLAN_

1) Turn on "QueryWeightFields" and set "QueryAutoTruncate" to auto
2) Turn off "UseQueryParser"
3) Do a keyword search in the OPAC
4) Note that the results are ordered by biblionumber ascending

5) Apply patch

6) Do the same keyword search in the OPAC
7) Note that the results are never ordered differently
(presumably in relevance order :P)

If you're a stickler, throw in some warns so that you can see
what CCL query is being sent to Zebra.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 73 Martin Renvoize 2020-12-03 11:14:58 UTC
Created attachment 114138 [details] [review]
Bug 12430: Add unit tests for C4::Search::buildQuery

This patch adds unit tests for buildQuery, especially focusing on
the use of the (rk=()) wrapper.

Note that the Net::Z3950::ZOOM library is used to test the queries
for CCL validity on top of the string matching used in the
got/expected comparisons.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2020-12-03 11:16:45 UTC
This one's been signed off a number of times and the final implementation appears to just move, and fix, the tests.. 

Setting to SO'd and QAing.
Comment 75 Martin Renvoize 2020-12-03 12:48:45 UTC
All works well and this is a really good bug to fix.. Passing QA
Comment 76 David Cook 2020-12-04 02:19:00 UTC
(In reply to Martin Renvoize from comment #75)
> All works well and this is a really good bug to fix.. Passing QA

Cheers, Martin :D
Comment 77 Jonathan Druart 2020-12-04 14:32:47 UTC
Created attachment 114200 [details] [review]
Bug 12430: Add unit tests for C4::Search::buildQuery

This patch adds unit tests for buildQuery, especially focusing on
the use of the (rk=()) wrapper.

Note that the Net::Z3950::ZOOM library is used to test the queries
for CCL validity on top of the string matching used in the
got/expected comparisons.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

JD amended patch: fix spelling connexion ==> connection
Comment 78 Jonathan Druart 2020-12-04 15:38:02 UTC
t/db_dependent/Search.t is failing, can you have a look please?
Comment 79 Jonathan Druart 2020-12-04 15:38:11 UTC
    #   Failed test 'buildQuery should keep 0 value'
    #   at t/db_dependent/Search.t line 669.
    #          got: 'su,phr=(rk=(0)) '
    #     expected: 'su,phr=0 '
Comment 80 Jonathan Druart 2020-12-04 16:02:56 UTC
Apparently the other failure is also coming from here.

t/db_dependent/selenium/regressions.t

    # Looks like you planned 4 tests but ran 3.
t/db_dependent/selenium/regressions.t .. 2/5 
#   Failed test 'OPAC - Remove from cart'
#   at t/db_dependent/selenium/regressions.t line 109.
Error while executing command: clickElement: An unknown server-side error occurred while processing the command.: Element is not clickable at point (583.7000122070312, 14). Other element would receive the click: <a class="page-link" href="/cgi-bin/koha/opac-search.pl?idx=kw&amp;q=d&amp;offset=420&amp;sort_by=relevance_dsc&amp;count=20" aria-label="Go to the last page"></a>
Comment 81 Jonathan Druart 2020-12-04 16:03:39 UTC Comment hidden (obsolete)
Comment 82 Jonathan Druart 2020-12-04 16:07:19 UTC
Created attachment 114205 [details] [review]
Bug 12430: Fix selenium/regressions.t failure

Test failed with
    # Looks like you planned 4 tests but ran 3.
t/db_dependent/selenium/regressions.t .. 2/5
 #   Failed test 'OPAC - Remove from cart'
 #   at t/db_dependent/selenium/regressions.t line 109.
Error while executing command: clickElement: An unknown server-side error occurred while processing the command.: Element is not clickable at point (583.7000122070312, 14). Other element would receive the click: <a class="page-link" href="/cgi-bin/koha/opac-search.pl?idx=kw&amp;q=d&amp;offset=420&amp;sort_by=relevance_dsc&amp;count=20" aria-label="Go to the last page"></a>

https://pic.infini.fr//Qhq7pLBq/jT9wZ7F1
The remove from basket link for the biblio 3 is hidden by the floating
toolbar.
Comment 83 David Cook 2020-12-06 23:49:42 UTC
(In reply to Jonathan Druart from comment #78)
> t/db_dependent/Search.t is failing, can you have a look please?

Is this still needed? And should it be on a separate bug?
Comment 84 Jonathan Druart 2020-12-07 16:55:03 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 85 Jonathan Druart 2020-12-07 16:55:23 UTC
(In reply to David Cook from comment #83)
> (In reply to Jonathan Druart from comment #78)
> > t/db_dependent/Search.t is failing, can you have a look please?
> 
> Is this still needed? And should it be on a separate bug?

Yes, still needed. My patch fixes another one.

Follow-up on this bug report is fine.
Comment 86 Jonathan Druart 2020-12-09 09:52:44 UTC
Created attachment 114275 [details] [review]
Bug 12430: Fix Search.t failure

t/db_dependent/Search.t ..
    #   Failed test 'buildQuery should keep 0 value'
    #   at t/db_dependent/Search.t line 669.
    #          got: 'su,phr=(rk=(0)) '
    #     expected: 'su,phr=0(rk=(0)) '
Comment 87 Jonathan Druart 2020-12-09 10:53:34 UTC
Last two patches pushed to master (21.05).
Comment 88 David Cook 2020-12-09 23:14:51 UTC
Thanks, Jonathan. I've been swamped so hadn't gotten around to it.
Comment 89 Fridolin Somers 2020-12-10 10:07:10 UTC
Pushed to 20.11.x for 20.11.01
Comment 90 Andrew Fuerste-Henry 2020-12-15 21:54:04 UTC
Pushed to 20.05.x for 20.05.07
Comment 91 Victor Grousset/tuxayo 2020-12-17 14:22:22 UTC
Backported to 19.11.x branch for 19.11.13
Comment 92 Victor Grousset/tuxayo 2020-12-17 18:18:59 UTC
Test plan works in 19.11.x but automated test doesn't [1] (t/Search/buildQuery.t)
It works in 20.05.x

Here is maybe the place to looks in the code. Does anyone understand the difference there or somewhere else that affect $limit_desc and $limit?

20.05.x:
https://gitlab.com/koha-community/Koha/-/blob/8a9553c4ae62aa05d7d3529806d8fdbac664b473/C4/Search.pm#L1587
19.11.x:
https://gitlab.com/koha-community/Koha/-/blob/608512179cf8f354ca76bab32e874530b3c5df12/C4/Search.pm#L1755


[1] Failed test:

kohadev-koha@ec7b568b5d3d:/kohadevbox/koha$ prove t/Search/buildQuery.t
t/Search/buildQuery.t .. 1/10 
    #   Failed test 'Limit is correct'
    #   at t/Search/buildQuery.t line 92.
    #          got: undef
    #     expected: ''

    #   Failed test 'Limit desc is correct'
    #   at t/Search/buildQuery.t line 94.
    #          got: undef
    #     expected: ''
    # Looks like you failed 2 tests of 13.

#   Failed test 'test weighted autotruncated'
#   at t/Search/buildQuery.t line 107.

    #   Failed test 'Limit is correct'
    #   at t/Search/buildQuery.t line 130.
    #          got: undef
    #     expected: ''

    #   Failed test 'Limit desc is correct'
    #   at t/Search/buildQuery.t line 132.
    #          got: undef
    #     expected: ''
    # Looks like you failed 2 tests of 13.

#   Failed test 'test* weighted autotruncated'
#   at t/Search/buildQuery.t line 144.

    #   Failed test 'Limit is correct'
    #   at t/Search/buildQuery.t line 167.
    #          got: undef
    #     expected: ''

    #   Failed test 'Limit desc is correct'
    #   at t/Search/buildQuery.t line 169.
    #          got: undef
    #     expected: ''
    # Looks like you failed 2 tests of 13.

#   Failed test 'test weighted not-autotruncated'
#   at t/Search/buildQuery.t line 181.

    #   Failed test 'Limit is correct'
    #   at t/Search/buildQuery.t line 316.
    #          got: undef
    #     expected: ''

    #   Failed test 'Limit desc is correct'
    #   at t/Search/buildQuery.t line 318.
    #          got: undef
    #     expected: ''
    # Looks like you failed 2 tests of 13.

#   Failed test 'one and two weighted autotruncated'
#   at t/Search/buildQuery.t line 330.
# Looks like you failed 4 tests of 10.
Comment 93 Victor Grousset/tuxayo 2020-12-17 18:27:09 UTC
It's likely that the tests should just expect undef instead of ''

But better ask for confirmation ^_^
Comment 94 Katrin Fischer 2023-07-28 20:11:44 UTC
*** Bug 13414 has been marked as a duplicate of this bug. ***