Bug 23521 - ES 6 - limit terms with many words can make the search inaccurate
Summary: ES 6 - limit terms with many words can make the search inaccurate
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Arnaud
QA Contact:
URL:
Keywords:
Depends on: 20196
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-29 14:50 UTC by Alex Arnaud
Modified: 2019-10-25 19:41 UTC (History)
5 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 23521: Put parentheses arround limit terms (3.03 KB, patch)
2019-08-29 15:06 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 23521: Put parentheses arround limit terms (3.23 KB, patch)
2019-10-04 10:59 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 23521: Put parentheses arround limit terms (3.30 KB, patch)
2019-10-25 13:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23521: (follow-up) Add quotes (3.03 KB, patch)
2019-10-25 13:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23521: Put parentheses arround limit terms (3.34 KB, patch)
2019-10-25 19:36 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 23521: (follow-up) Add quotes (3.07 KB, patch)
2019-10-25 19:36 UTC, Ere Maijala
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2019-08-29 14:50:36 UTC
That issue seems to happen only with elasticsearch 6. So make this BZ depends on ES 6 Omnibus.

Refining a search on author lead to the following query:

    "query_string": {
      "query": "(_record:*) AND author:Dillinger Girl"
    }

This query returns a document with "Dillinger Escaplan" as author and "Girl" in the title => Ko

The fix should be something like:

    "query_string": {
      "query": "(_record:*) AND author:(Dillinger Girl)"
    }

That query returns only documents with "Dillinger Girl" as author => Ok
Comment 1 Alex Arnaud 2019-08-29 15:06:58 UTC
Created attachment 92498 [details] [review]
Bug 23521: Put parentheses arround limit terms

Test plan:
  - Use Elasticsearch 6 (you'll need Bug 18969),
  - create a biblio (#1) with "Dillinger Girl" in author and what you
    want in title,
  - create another biblio (#2) with the word "girl" in the title and
    "Dillinger Escaplan" as author
  - reindex
  - search * and refine on "Dillinger Girl"
  - Ko => Biblio #1 and #2 appear
  - Apply this patch,
  - search * and refine on "Dillinger Girl"
  - Ok => anly biblio #1 appears
  - use Elasticsearch 5 again
  - check for no search regression
Comment 3 Fridolin SOMERS 2019-10-03 14:23:11 UTC
(In reply to Fridolin SOMERS from comment #2)
> See doc :
> 
> https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-query-
> string-query.html

You see an example of query containing parenthesis ofr terms with many words
Comment 4 sandboxes@biblibre.com 2019-10-04 10:59:10 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 5 sandboxes@biblibre.com 2019-10-04 10:59:31 UTC
Created attachment 93741 [details] [review]
Bug 23521: Put parentheses arround limit terms

Test plan:
  - Use Elasticsearch 6 (you'll need Bug 18969),
  - create a biblio (#1) with "Dillinger Girl" in author and what you
    want in title,
  - create another biblio (#2) with the word "girl" in the title and
    "Dillinger Escaplan" as author
  - reindex
  - search * and refine on "Dillinger Girl"
  - Ko => Biblio #1 and #2 appear
  - Apply this patch,
  - search * and refine on "Dillinger Girl"
  - Ok => anly biblio #1 appears
  - use Elasticsearch 5 again
  - check for no search regression

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 6 Nick Clemens 2019-10-25 13:55:39 UTC
Created attachment 94771 [details] [review]
Bug 23521: Put parentheses arround limit terms

Test plan:
  - Use Elasticsearch 6 (you'll need Bug 18969),
  - create a biblio (#1) with "Dillinger Girl" in author and what you
    want in title,
  - create another biblio (#2) with the word "girl" in the title and
    "Dillinger Escaplan" as author
  - reindex
  - search * and refine on "Dillinger Girl"
  - Ko => Biblio #1 and #2 appear
  - Apply this patch,
  - search * and refine on "Dillinger Girl"
  - Ok => anly biblio #1 appears
  - use Elasticsearch 5 again
  - check for no search regression

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2019-10-25 13:55:42 UTC
Created attachment 94772 [details] [review]
Bug 23521: (follow-up) Add quotes

Not only should terms from facets/limits be grouped, but order should probably matter
We build a different facet for "Dillinger Girl" or "Girl Dillinger"

The parens are possibly overkill now, but I think it makes it very clear and does not hurt

To test:
1 - Follow original plan
2 - Create a new record with 'Girl Dillinger' as author
3 - Search for 'Dill*'
4 - You get all three records (and maybe others that match)
5 - Limit by 'Girl Dillinger' - you get two records
6 - Same for 'Dillinger Girl'
7 - Apply patch
8 - Limits/facets for 'Dillinger Girl' and 'Girl Dillinger' now match a isngle record
Comment 8 Nick Clemens 2019-10-25 13:56:34 UTC
Alex, your patches work well, but I took things a bit further, can you check my work? Added Ere to CC for another opinion.
Comment 9 Ere Maijala 2019-10-25 19:36:49 UTC
Created attachment 94799 [details] [review]
Bug 23521: Put parentheses arround limit terms

Test plan:
  - Use Elasticsearch 6 (you'll need Bug 18969),
  - create a biblio (#1) with "Dillinger Girl" in author and what you
    want in title,
  - create another biblio (#2) with the word "girl" in the title and
    "Dillinger Escaplan" as author
  - reindex
  - search * and refine on "Dillinger Girl"
  - Ko => Biblio #1 and #2 appear
  - Apply this patch,
  - search * and refine on "Dillinger Girl"
  - Ok => anly biblio #1 appears
  - use Elasticsearch 5 again
  - check for no search regression

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 10 Ere Maijala 2019-10-25 19:36:53 UTC
Created attachment 94800 [details] [review]
Bug 23521: (follow-up) Add quotes

Not only should terms from facets/limits be grouped, but order should probably matter
We build a different facet for "Dillinger Girl" or "Girl Dillinger"

The parens are possibly overkill now, but I think it makes it very clear and does not hurt

To test:
1 - Follow original plan
2 - Create a new record with 'Girl Dillinger' as author
3 - Search for 'Dill*'
4 - You get all three records (and maybe others that match)
5 - Limit by 'Girl Dillinger' - you get two records
6 - Same for 'Dillinger Girl'
7 - Apply patch
8 - Limits/facets for 'Dillinger Girl' and 'Girl Dillinger' now match a isngle record

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 11 Ere Maijala 2019-10-25 19:41:49 UTC
Makes sense to me. I don't particularly like the slightly ambiguous syntax used with limits (I'd interpret 'author:Something Other' as 'author:(Something) AND Other', but since it works with Zebra, we need to support that.

A helper function for splitting a string to field and term would be nice since we do the same regexp now in at least two places, but it's still clear enough that it works for me.