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: RESOLVED FIXED
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: 25882
  Show dependency treegraph
 
Reported: 2019-08-29 14:50 UTC by Alex Arnaud
Modified: 2020-11-30 21:52 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05


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.
Comment 12 Martin Renvoize 2020-03-11 14:13:50 UTC
With comments (and subsequent signoffs) from all ES experts already I'm going to go for PQA on this one.
Comment 13 Martin Renvoize 2020-03-16 11:03:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-04-03 17:24:26 UTC
Backported to 19.11.x branch for 19.11.05
Comment 15 Lucas Gass 2020-04-10 15:37:51 UTC
missing dependencies for 19.05.x, not backporting