Refactor simple_query_string to handle text part like multi_match and query_string#26145
Conversation
dakrone
left a comment
There was a problem hiding this comment.
I left some comments on this, thanks for tackling this!
The
use_all_fieldsoption is deprecated in this change and can be replaced by setting*in the fields parameter.
I think you mean the all_fields option?
There was a problem hiding this comment.
this is technically breaking, because prior to this change the following would work:
builder.field("foo", 1.5);
builder.useAllFields(true);
builder.useAllFields(false);And since setting it to true permanently changes the fieldsAndWeights. While it seems contrived, I could see user code doing this passing the builder off to independent functions, the builder should be idempotent in this case
There was a problem hiding this comment.
It also breaks what I would expect to happen with:
builder.useAllFields(true);
builder.field("foo", 1.0);Where I would expect it to use all_fields mode, but instead it does not work since the order matters.
There was a problem hiding this comment.
In the latter case it would fail the query early because useAllFields freeze the fieldsAndWeights map. I think it's better than the current behavior which accepts this query but fails when the query is actually built (fields and useAllFields are mutually exclusive) ?
For the first example I don't know if it's really a problem. Maybe we should just disallow useAllFields(false) ? I don't see a good reason to unset it especially now that the index default field defaults to * ?
There was a problem hiding this comment.
In the latter case it would fail the query early because useAllFields freeze the fieldsAndWeights map. I think it's better than the current behavior which accepts this query but fails when the query is actually built (fields and useAllFields are mutually exclusive) ?
Sure, that sounds fine, as long as we're okay with this failing (esoteric) use-case.
For the first example I don't know if it's really a problem. Maybe we should just disallow useAllFields(false) ? I don't see a good reason to unset it especially now that the index default field defaults to * ?
Yeah, I don't either, the only argument I can think of keeping useAllFields(boolean) is that then it's apparent it's a "setter" and not a "getter"
There was a problem hiding this comment.
I believe this does need to calculate whether all fields mode is going to be used, otherwise a mixed 5.6 - 6.0 cluster will have the behavior correct on 6.0 and incorrect on 5.6, since 5.6 calculates the decision for whether to use all_fields using different criteria than 6.0 will?
There was a problem hiding this comment.
Can you add javadocs for this class please?
There was a problem hiding this comment.
And also javadocs for all methods in this class too please
There was a problem hiding this comment.
I think this no longer needs to be fully qualified since the class name is not the same any more
|
Thanks for looking @dakrone
Yes, I don't know why but I always get the name wrong ;) |
… query_string This change is a continuation of elastic#25726 that aligns field expansions for the simple_query_string with the query_string and multi_match query. The main changes are: * For exact field name, the new behavior is to rewrite to a matchnodocs query when the field name is not found in the mapping. * For partial field names (with * suffix), the expansion is done only on keyword, text, date, ip and number field types. Other field types are simply ignored. * For all fields (*), the expansion is done on accepted field types only (see above) and metadata fields are also filtered. The use_all_fields option is deprecated in this change and can be replaced by setting `*` in the fields parameter. This commit also changes how text fields are analyzed. Previously the default search analyzer (or the provided analyzer) was used to analyze every text part , ignoring the analyzer set on the field in the mapping. With this change, the field analyzer is used instead unless an analyzer has been forced in the parameter of the query. Finally now that all full text queries can handle the special "*" expansion (`all_fields` mode), the `index.query.default_field` is now set to `*` for indices created in 6.
456ceac to
28d92b6
Compare
… query_string (#26145) This change is a continuation of #25726 that aligns field expansions for the simple_query_string with the query_string and multi_match query. The main changes are: * For exact field name, the new behavior is to rewrite to a matchnodocs query when the field name is not found in the mapping. * For partial field names (with * suffix), the expansion is done only on keyword, text, date, ip and number field types. Other field types are simply ignored. * For all fields (*), the expansion is done on accepted field types only (see above) and metadata fields are also filtered. The use_all_fields option is deprecated in this change and can be replaced by setting `*` in the fields parameter. This commit also changes how text fields are analyzed. Previously the default search analyzer (or the provided analyzer) was used to analyze every text part , ignoring the analyzer set on the field in the mapping. With this change, the field analyzer is used instead unless an analyzer has been forced in the parameter of the query. Finally now that all full text queries can handle the special "*" expansion (`all_fields` mode), the `index.query.default_field` is now set to `*` for indices created in 6.
… query_string (#26145) This change is a continuation of #25726 that aligns field expansions for the simple_query_string with the query_string and multi_match query. The main changes are: * For exact field name, the new behavior is to rewrite to a matchnodocs query when the field name is not found in the mapping. * For partial field names (with * suffix), the expansion is done only on keyword, text, date, ip and number field types. Other field types are simply ignored. * For all fields (*), the expansion is done on accepted field types only (see above) and metadata fields are also filtered. The use_all_fields option is deprecated in this change and can be replaced by setting `*` in the fields parameter. This commit also changes how text fields are analyzed. Previously the default search analyzer (or the provided analyzer) was used to analyze every text part , ignoring the analyzer set on the field in the mapping. With this change, the field analyzer is used instead unless an analyzer has been forced in the parameter of the query. Finally now that all full text queries can handle the special "*" expansion (`all_fields` mode), the `index.query.default_field` is now set to `*` for indices created in 6.
This change is a continuation of #25726 that aligns field expansions and text analysis for the
simple_query_stringwith thequery_stringandmulti_match query.The main changes are:
For exact field name, the new behavior is to rewrite to a matchnodocs query when the field name is not found in the mapping.
For partial field names (with * suffix), the expansion is done only on keyword, text, date, ip and number field types. Other field types are simply ignored.
For all fields (*), the expansion is done on accepted field types only (see above) and metadata fields are also filtered.
The
use_all_fields optionis deprecated in this change and can be replaced by setting*in the fields parameter.This commit also changes how text fields are analyzed. Previously the default search analyzer (or the provided analyzer) was used to analyze every text part
, ignoring the analyzer set on the field in the mapping. With this change, the field analyzer is used instead unless an analyzer has been forced in the parameter of the query.
Finally now that all full text queries can handle the special "*" expansion (
all_fieldsmode), theindex.query.default_fieldis now set to*for indices created in 6.