Bug 11137 - QueryParser can ignore parts of a Boolean search after the first
Summary: QueryParser can ignore parts of a Boolean search after the first
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Galen Charlton
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-24 20:16 UTC by Galen Charlton
Modified: 2015-06-04 23:24 UTC (History)
2 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 11137: regression tests for QP search field alias bug (4.96 KB, patch)
2013-10-24 20:54 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11137: fix certain types of searches run using QueryParser (2.66 KB, patch)
2013-10-24 20:54 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11137: regression tests for QP search field alias bug (4.96 KB, patch)
2013-10-24 21:33 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11137: fix certain types of searches run using QueryParser (2.91 KB, patch)
2013-10-24 21:33 UTC, Galen Charlton
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11137: regression tests for QP search field alias bug (5.03 KB, patch)
2013-10-27 03:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11137: fix certain types of searches run using QueryParser (3.13 KB, patch)
2013-10-27 03:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11137: regression tests for QP search field alias bug (4.99 KB, patch)
2013-11-14 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11137: fix certain types of searches run using QueryParser (3.86 KB, patch)
2013-11-14 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2013-10-24 20:16:04 UTC
The way that the QueryParser configuration is initialized results in duplicate search field aliases being defined, which in turn can cause problems with Boolean searches.

For example, consider the following search:

kw:history && earth

This will retrieve all records that have both 'history' and 'earth' as keywords.

Depending on your search settings, this search is received by Zebra as

@attrset Bib-1 @and @attr 4=6 @attr 5=1 @attr 1=1016 history @attr 4=6 @attr 5=1 @attr 1=1016 earth

Now consider this search, which should be identical:

kw:history && kw:earth

This search retrieves all records containing 'history', but ignores 'earth'.  The search received by Zebra is:

@attrset Bib-1 @attr 4=6 @attr 5=1 @attr 1=1016 histor
Comment 1 Galen Charlton 2013-10-24 20:54:34 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-10-24 20:54:40 UTC Comment hidden (obsolete)
Comment 3 Galen Charlton 2013-10-24 21:33:40 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2013-10-24 21:33:50 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2013-10-27 03:57:46 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2013-10-27 03:57:57 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-11-14 14:28:03 UTC
I cannot test this patch. My MARC21 DB is broken and with my UNIMARC DB, I got no result.

I tried to debug:

In QueryParser::Driver::PQF::target_syntax, debug is enable.

- with marcflavour=MARC21
QP query for biblioserver:  history #relevance_dsc 
PQF query: @attr 2=102  @or  @or @attr 1=1016 @attr 5=1  @attr 4=6  "history"   @attr 9=20  @attr 2=102 @attr 5=1  @attr 4=6  "history"   @attr 9=34  @attr 2=102 @attr 5=1  @attr 4=6  "history"

- with marcflavour=UNIMARC
 QP query for biblioserver:  history #relevance_asc 
 PQF query:  @and  @or  @or @attr 1=1016 @attr 4=6  "history"   @attr 9=20  @attr 2=102 @attr 4=6  "history"   @attr 9=34  @attr 2=102 @attr 4=6  "history"   @or  @or @attr 1=1016 @attr 4=6  "#relevance_asc"   @attr 9=20  @attr 2=102 @attr 4=6  "#relevance_asc"   @attr 9=34  @attr 2=102 @attr 4=6  "#relevance_asc"

The call to $self->parse_tree->target_syntax does not remove the modifiers for UNIMARC. I don't know how continue to investigate.
Comment 8 Jonathan Druart 2013-11-14 14:37:34 UTC
So stupid...
My Unimarc DB got a default sort field relevance dsc and there is no modifier relevance dsc in the qp.yml file.
Comment 9 Jonathan Druart 2013-11-14 14:47:29 UTC
Created attachment 22932 [details] [review]
Bug 11137: regression tests for QP search field alias bug

This patch adds regression tests to verify that
Boolean searches using QueryParser function correctly.

This patch also ensures that QP is correctly initalized
when Search.t is run.

To test:

[1] Apply this patch and the following patch.
[2] Verify that prove -v t/QueryParser.t works
[3] Verify that prove -v t/db_dependent/Search.t works
[4] (optional) instead of applying both patches at the same
    time, apply only the regression test patch and run the
    tests listed in steps 2 and 3.  The following tests
    should fail:

t/db_dependent/Search.t (Wstat: 512 Tests: 198 Failed: 2)
  Failed tests:  42, 71
  Non-zero exit status: 2
t/QueryParser.t (Wstat: 256 Tests: 28 Failed: 1)
  Failed test:  12
  Non-zero exit status: 1

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Jonathan Druart 2013-11-14 14:47:56 UTC
Created attachment 22933 [details] [review]
Bug 11137: fix certain types of searches run using QueryParser

QueryParser was loading redundant search aliases, which in turn resulted
in certain queries involving two or more clauses joined by a Boolean
operator getting parsed in correctly.

To test:

[1] Enable QueryParser.
[2] Arrange your database so that some some bib records contain
    the keyword 'history' while a smaller subset contain
    both 'history' and 'earth'.  (The exact words used don't
    matter.)
[3] Run the following search from the OPAC global search bar:

    kw:history && kw:earth

[4] Observe that the records that are returned include *all* bibs
    in the database with the keyword 'history'.  In other words, the
    restriction  that the records must also contain 'earth' is not
    observed.
[5] Apply the patch and rerun the search.
[6] This time, the results that are returned should include only those
    that contain both 'history' and 'earth'.
[7] Verify that prove -v t/QueryParser.t passes.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes test plans, all tests and QA script.
Also checked the db_dependent/Search.t tests.
Tried some other searches, all seems to be working correctly.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I reproduce the bug and I confirm this patch fixes it.
Without the patch, kw:history && kw:earth, I got:
  Search biblios OK 4 1 1+0 RPN @attrset Bib-1 @attr 4=6 @attr 1=1016 history
With the path, I get:
   Search biblios OK 2 1 1+0 RPN @attrset Bib-1 @and @attr 4=6 @attr 1=1016 history @attr 4=6 @attr 1=1016 earth
Comment 11 Jonathan Druart 2013-11-14 14:51:26 UTC
Galen: I know there is no sense to sort by relevance asc but don't you know it is worth to add an entry in the qp config file?
Comment 12 Galen Charlton 2013-11-14 15:48:52 UTC
(In reply to Jonathan Druart from comment #11)
> Galen: I know there is no sense to sort by relevance asc but don't you know
> it is worth to add an entry in the qp config file?

I think so, as there's nothing preventing somebody from setting relevance ascending as the default sort order.
Comment 13 Galen Charlton 2013-11-14 15:54:01 UTC
Pushed to master and 3.14.x.
Comment 14 Jonathan Druart 2013-11-15 10:17:52 UTC
(In reply to Galen Charlton from comment #12)
> (In reply to Jonathan Druart from comment #11)
> > Galen: I know there is no sense to sort by relevance asc but don't you know
> > it is worth to add an entry in the qp config file?
> 
> I think so, as there's nothing preventing somebody from setting relevance
> ascending as the default sort order.

Patch submitted in bug 11255.
Comment 15 Tomás Cohen Arazi 2013-12-08 15:08:21 UTC
This patch has been pushed to 3.12.x, will be in 3.12.8.

Thanks Galen!