-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Fix match_only_text keyword multi-field bug #131383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix match_only_text keyword multi-field bug #131383
Conversation
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- match: { "hits.total.value": 1 } | ||
- match: | ||
hits.hits.0._source.foo: "Apache Lucene powers Elasticsearch" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to convince myself that there won't ever be duplicate values when some values come from doc_values and some from the original field. It might be nice to have a test that covers this case. Something like:
synthetic_source match_only_text as multi-field with ignored stored keyword as parent with multiple values:
- do:
indices.create:
index: synthetic_source_test
body:
settings:
index:
mapping.source.mode: synthetic
mappings:
properties:
foo:
type: keyword
store: false
doc_values: true
ignore_above: 10
fields:
text:
type: match_only_text
- do:
index:
index: synthetic_source_test
id: "1"
refresh: true
body:
foo: ["Apache Lucene powers Elasticsearch", "Apache"]
- do:
search:
index: synthetic_source_test
body:
query:
match_phrase:
foo.text: apache lucene
- match: { "hits.total.value": 1 }
- match:
hits.hits.0._source.foo: ["Apache", "Apache Lucene powers Elasticsearch"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one test suggestion, but looks good. Nice work!
16a8e7f
to
ea6e60f
Compare
In elastic#131314 we fixed match_only_text fields with ignore_above keyword multi-fields in the case that the keyword multi-field is stored. However, the issue is still present if the keyword field is not stored, but instead has doc values. This patch fixes that case.
In elastic#131314 we fixed match_only_text fields with ignore_above keyword multi-fields in the case that the keyword multi-field is stored. However, the issue is still present if the keyword field is not stored, but instead has doc values. This patch fixes that case.
In #131314 we fixed match_only_text fields with ignore_above keyword multi-fields in the case that the keyword multi-field is stored. However, the issue is still present if the keyword field is not stored, but instead has doc values. This patch fixes that case.
In #131314 we fixed match_only_text fields with ignore_above keyword multi-fields in the case that the keyword multi-field is stored. However, the issue is still present if the keyword field is not stored, but instead has doc values. This patch fixes that case.
In #131314 we fixed match_only_text fields with ignore_above keyword multi-fields in the case that the keyword multi-field is stored. However, the issue is still present if the keyword field is not stored, but instead has doc values.
This patch fixes that case.
Follow-up to #131314.