-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
search as you type fieldmapper #35600
search as you type fieldmapper #35600
Conversation
this verifies that all the fields are correctly configured in the default case and that all the fields are accounted for
max shingles = 3 by default, and min gram and max gram are no longer configurable
Pinging @elastic/es-search-aggs |
@jimczi from our discussion yesterday I
I think this is ready for a first look unless there's anything else you'd like me to add |
...rch-as-you-type/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java
Outdated
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.
It's a great start @andyb-elastic . I left some comments regarding how we should interact with this new mapper.
server/src/main/java/org/apache/lucene/index/FreqIsScoreSimilarity.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/OldSearchAsYouTypeFieldMapper.java
Outdated
Show resolved
Hide resolved
...rch-as-you-type/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java
Outdated
Show resolved
Hide resolved
...rch-as-you-type/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java
Outdated
Show resolved
Hide resolved
...rch-as-you-type/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java
Outdated
Show resolved
Hide resolved
...s-you-type/src/test/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapperTests.java
Outdated
Show resolved
Hide resolved
...rch-as-you-type/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java
Outdated
Show resolved
Hide resolved
...rch-as-you-type/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java
Outdated
Show resolved
Hide resolved
maxShingleSize = XContentMapValues.nodeIntegerValue(fieldNode); | ||
iterator.remove(); | ||
} | ||
} |
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.
Should we throw an error if the map is not empty (has some fields that we don't support) ?
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 assumed whatever calls this method did that afterwards, it looks like in at least some places they do. Should I throw one here too?
elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
Lines 276 to 284 in 3c03975
Mapper.Builder<?,?> fieldBuilder = typeParser.parse(realFieldName, propNode, parserContext); | |
for (int i = fieldNameParts.length - 2; i >= 0; --i) { | |
ObjectMapper.Builder<?, ?> intermediate = new ObjectMapper.Builder<>(fieldNameParts[i]); | |
intermediate.add(fieldBuilder); | |
fieldBuilder = intermediate; | |
} | |
objBuilder.add(fieldBuilder); | |
propNode.remove("type"); | |
DocumentMapperParser.checkNoRemainingFields(fieldName, propNode, parserContext.indexVersionCreated()); |
tests pass
tests pass
server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.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.
Thanks for writing the docs @andyb-elastic , I left some comments.
When going through what multi match parameters are supported for bool_prefix, it looks like the operator, auto_generate_synonyms_phrase_query, and the fuzziness family of settings are applied to query in the multi_match context, so it seems like we should support them in the single-query form too
I agree, we should at least support operator
and fuzziness
.
server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/query/BoolPrefixQueryBuilder.java
Outdated
Show resolved
Hide resolved
Sounds good, I think I'd like to add them in a follow up |
Sure as you like. Let's not forget it though ;) |
I'm adding those parameters here since it's not a big change |
I pushed changes that rename the query to |
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.
This change looks good to me, let's merge !
Adds the search_as_you_type field type that acts like a text field optimized for as-you-type search completion. It creates a couple subfields that analyze the indexed terms as shingles, against which full terms are queried, and a prefix subfield that analyze terms as the largest shingle size used and edge-ngrams, against which partial terms are queried Adds a match_bool_prefix query type that creates a boolean clause of a term query for each term except the last, for which a boolean clause with a prefix query is created. The match_bool_prefix query is the recommended way of querying a search as you type field, which will boil down to term queries for each shingle of the input text on the appropriate shingle field, and the final (possibly partial) term as a term query on the prefix field. This field type also supports phrase and phrase prefix queries however
Adds the search_as_you_type field type that acts like a text field optimized for as-you-type search completion. It creates a couple subfields that analyze the indexed terms as shingles, against which full terms are queried, and a prefix subfield that analyze terms as the largest shingle size used and edge-ngrams, against which partial terms are queried Adds a match_bool_prefix query type that creates a boolean clause of a term query for each term except the last, for which a boolean clause with a prefix query is created. The match_bool_prefix query is the recommended way of querying a search as you type field, which will boil down to term queries for each shingle of the input text on the appropriate shingle field, and the final (possibly partial) term as a term query on the prefix field. This field type also supports phrase and phrase prefix queries however
- match: { hits.total: 1 } | ||
- match: { hits.hits.0._source.a_field: "quick brown fox jump lazy dog" } | ||
- match: { hits.hits.0._source.text_field: "quick brown fox jump lazy dog" } | ||
- match: { hits.hits.0.highlight.a_field.0: "quick <em>brown</em> fox jump lazy dog" } |
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.
@andyb-elastic Is it unreasonable to expect quick <em>brown</em> <em>fox</em> jump lazy dog
highlight here?
(That's what match_bool_prefix: "brown fo"
on text_field
would give).
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 know it's not nice to comment on already merged PR, that's why I opened this issue:
#53744
Adds a new field type,
search_as_you_type
, that acts like a text field optimized for as-you-type search completion. It creates a couple subfields that analyze the indexed terms as shingles, against which full terms are queried, and a prefix subfield that analyze terms as the largest shingle size used and edge-ngrams, against which partial terms are queriedAdds a
boolean_prefix
query type that creates a boolean clause of a term query for each term except the last, for which a boolean clause with a prefix query is created. This will be used as the recommended query type for queryingsearch_as_you_type
fields, although other text queries will be supported as wellThis is for #33160