Bug 27070 - Elasticsearch - with Elasticsearch 6 searches failing unless all terms are in the same field
Summary: Elasticsearch - with Elasticsearch 6 searches failing unless all terms are in...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 27252
  Show dependency treegraph
 
Reported: 2020-11-20 19:09 UTC by Nick Clemens
Modified: 2022-07-08 11:47 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07, 19.11.14


Attachments
Bug 27070: Add cross_fields type to our searches (1.28 KB, patch)
2020-11-20 19:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27070: Add cross_fields type to our searches (1.82 KB, patch)
2020-11-20 19:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27070: Add cross_fields type to our searches (1.34 KB, patch)
2020-11-21 13:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27070: Add cross_fields type to our searches (1.39 KB, patch)
2020-11-21 13:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27070: Add cross_fields type to our searches (1.45 KB, patch)
2020-11-21 18:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27070: Add cross_fields type to our searches (1.60 KB, patch)
2020-11-25 12:45 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-11-20 19:09:50 UTC
To recreate - be using Elasticsearch 6 in KOha

On sample data try searching for:
street shuffle

cool, that works

Now try searching for a word from title and the publisher name:
shuffle constable

No results, odd.

Try searching for:
shuffle AND constable

That works.

I cannot figure from the docs why this is the case - we pass a default operator, that should be enough I thought, but no.

https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-query-string-query.html#query-string-syntax

for comparison:
https://www.elastic.co/guide/en/elasticsearch/reference/5.6/query-dsl-query-string-query.html


After testing the type => 'cross_fields' seems to do what we want
Comment 1 Nick Clemens 2020-11-20 19:11:32 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2020-11-20 19:14:20 UTC Comment hidden (obsolete)
Comment 3 Didier Gautheron 2020-11-21 12:36:00 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2020-11-21 13:17:03 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2020-11-21 13:17:59 UTC
Created attachment 113908 [details] [review]
Bug 27070: Add cross_fields type to our searches

This patch adds the 'cross_fields' type to our searches:
https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-query-string-query.html#query-string-syntax

Without this patch the search terms seem to all require being in the same field when using Elasticsearch 6

To test:
0 - Set QueryAutoTruncate to 'only if * is added'
1 - Find a record with a title and publisher
2 - Search for a word form the title and confirm the record is returned
3 - Search for a work from the title and the publisher's name
4 - The record is not returned
5 - Apply patch
6 - Repeat #3
7 - The record is returned
Comment 6 Victor Grousset/tuxayo 2020-11-21 18:59:43 UTC
Created attachment 113911 [details] [review]
Bug 27070: Add cross_fields type to our searches

This patch adds the 'cross_fields' type to our searches:
https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-query-string-query.html#query-string-syntax

Without this patch the search terms seem to all require being in the same field when using Elasticsearch 6

To test:
0 - Set QueryAutoTruncate to 'only if * is added'
1 - Find a record with a title and publisher
2 - Search for a word form the title and confirm the record is returned
3 - Search for a work from the title and the publisher's name
4 - The record is not returned
5 - Apply patch
6 - Repeat #3
7 - The record is returned

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Joonas Kylmälä 2020-11-24 12:58:41 UTC
Any idea how well this works with weighted search fields? There was some discussion in https://discuss.elastic.co/t/cross-fields-and-boost-is-it-feasible/134219/9 about it and also https://www.elastic.co/guide/en/elasticsearch/reference/master/query-dsl-multi-match-query.html#type-cross-fields mentions it but I don't see any immediate issues in our use case.
Comment 8 Fridolin Somers 2020-11-24 13:18:57 UTC
Whooo I was looking for this fix since weeks, thanks a lot
Comment 9 Nick Clemens 2020-11-24 13:51:04 UTC
(In reply to Joonas Kylmälä from comment #7)
> Any idea how well this works with weighted search fields? There was some
> discussion in
> https://discuss.elastic.co/t/cross-fields-and-boost-is-it-feasible/134219/9
> about it and also
> https://www.elastic.co/guide/en/elasticsearch/reference/master/query-dsl-
> multi-match-query.html#type-cross-fields mentions it but I don't see any
> immediate issues in our use case.

It seems mostly that they are saying cross_fields does more than just searching all fields, it tries to guess which fields are most relevant to each term and boost them - so adding field boosts on top of that kind of ruins the work.

The problem I see is that the multi field query says it should look for each term in each field with 'OR', i.e.
(content:this OR name:this) AND (content:that OR name:that)
but what is returned actually seems to be:
(content:this AND that) AND (name:this AND that)

possibly the problem is with our 'default_operator' setting of 'AND'

I think with OR we might end up with results that don't contain all terms, but the ones that do should be most relevant, but our field boosting may push partial matches to the top.

cross_fields seems to at least generate the search we expect right now. I am open to discussion, but the current state here is problematic for many users
Comment 10 Joonas Kylmälä 2020-11-25 12:45:04 UTC
Created attachment 113976 [details] [review]
Bug 27070: Add cross_fields type to our searches

This patch adds the 'cross_fields' type to our searches:
https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-query-string-query.html#query-string-syntax

Without this patch the search terms seem to all require being in the same field when using Elasticsearch 6

To test:
0 - Set QueryAutoTruncate to 'only if * is added'
1 - Find a record with a title and publisher
2 - Search for a word form the title and confirm the record is returned
3 - Search for a work from the title and the publisher's name
4 - The record is not returned
5 - Apply patch
6 - Repeat #3
7 - The record is returned

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 11 Joonas Kylmälä 2020-11-25 12:49:30 UTC
Using cross_fields here makes this now so that it doesn't try to do an AND search on one field only for all the search keywords which is pretty much guaranteed to fail as people here noticed. The searches now at least bring all the results that contain those keywords, when previously very much relevant results were missing! I'm passing QA for this given it is a big improvement to the previous state but in the future we still need to verify and read the Elasticsearch docs if there could be a even better way doing searches.
Comment 12 Jonathan Druart 2020-11-25 13:07:10 UTC
Joonas, could you open a follow-up bug report with the links/ideas to explore?
Comment 13 Joonas Kylmälä 2020-11-25 13:30:35 UTC
(In reply to Jonathan Druart from comment #12)
> Joonas, could you open a follow-up bug report with the links/ideas to
> explore?

I opened bug 27091. It's gonna be tedious process but I'm actually planning to start doing in the following 3 months, hopefully also being ready then :)
Comment 14 Jonathan Druart 2020-11-25 15:20:07 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 15 Andrew Fuerste-Henry 2020-12-14 15:19:49 UTC
Pushed to 20.05.x for 20.05.07
Comment 16 Nick Clemens 2020-12-14 16:08:17 UTC
There is an issue here, this breaks searching with Elasticsearch 5.x

It may be okay, ES5 is no longer supported, so this may be the point at which we want to support only Es6+, but we will need to announce that
Comment 17 Victor Grousset/tuxayo 2020-12-14 18:30:07 UTC
EOL since 2019-03-11 IIUC
https://www.elastic.co/support/eol
Comment 18 Jonathan Druart 2020-12-16 13:12:48 UTC
(In reply to Nick Clemens from comment #16)
> There is an issue here, this breaks searching with Elasticsearch 5.x
> 
> It may be okay, ES5 is no longer supported, so this may be the point at
> which we want to support only Es6+, but we will need to announce that

In that case I am suggesting to remove it from 20.05, before .7 is released.
Comment 19 Jonathan Druart 2020-12-16 13:15:43 UTC
And add a warning on the about page, see bug 27252.
Comment 20 Victor Grousset/tuxayo 2020-12-16 17:49:10 UTC
Does this mean that for now, 19.11 and 20.05 will have to advertise (and test in the CI) ES 5 only?
Comment 21 Victor Grousset/tuxayo 2021-01-17 21:56:00 UTC
> Does this mean that for now, 19.11 and 20.05 will have to advertise (and test in the CI) ES 5 only?

Nope, they can support ES 6 and ES 5 thanks to bug 27252
Comment 22 Victor Grousset/tuxayo 2021-01-17 21:58:06 UTC
Backported: Pushed to 19.11.x branch for 19.11.14