Bug 12430 - Search truncation disables relevance ranking
Summary: Search truncation disables relevance ranking
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Zebra (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: David Cook
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-17 02:50 UTC by David Cook
Modified: 2020-03-02 02:31 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12430 - Truncation disables relevance ranking when not using QueryParser (1.98 KB, patch)
2014-06-17 07:17 UTC, David Cook
Details | Diff | Splinter Review
Bug 12430 - Truncation disables relevance ranking when not using QueryParser (2.27 KB, patch)
2019-12-10 17:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12430 - Truncation disables relevance ranking when not using QueryParser (2.34 KB, patch)
2020-01-30 22:07 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12430 - Truncation disables relevance ranking when not using QueryParser (2.45 KB, patch)
2020-02-09 12:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12430: Force relevance ranking when not using QueryParser (2.43 KB, patch)
2020-02-18 02:38 UTC, David Cook
Details | Diff | Splinter Review
First Draft buildQuery unit test (3.10 KB, patch)
2020-02-18 03:03 UTC, David Cook
Details | Diff | Splinter Review
Unit tests for buildQuery (12.01 KB, patch)
2020-02-18 08:34 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.