-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Change the way that fields are selected for inclusion in all
queries.
#31655
Conversation
Currently this is a hardcoded whitelist, which is problematic due to the fact that plugins can't add new types to this list, we already have the problem with the `scaled_float` type today. This change proposes to rely on exception types of the `termQuery` factory method instead to know whether a field should be searched. Another option would be to add an API to MappedFieldType to check whether a field must be considered for `all` queries, but I wanted to avoid adding a new API.
Pinging @elastic/es-search-aggs |
* @throws ElasticsearchParseException if {@code value} cannot be converted to the expected data type | ||
* @throws UnsupportedOperationException if the field is not searchable regardless of options | ||
* @throws QueryShardException if the field is not searchable regardless of options | ||
*/ |
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.
The last 2 javadoc "throws" descriptions give identical reasons?
// Unmapped fields are not ignored | ||
fields.put(fieldOrPattern, weight); | ||
continue; | ||
} | ||
if (acceptMetadataField == false && mapper instanceof MetadataFieldMapper) { | ||
if (acceptMetadataField == false && ft.name().startsWith("_")) { | ||
// Ignore metadata fields |
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.
Would this break BWC for some users? Underscored fieldnames were always by convention "ours" but are we now being stronger on this (either with docs or enforcing naming conventions?)
continue; | ||
} catch (RuntimeException e) { | ||
// other exceptions are parsing errors or not indexed fields: keep | ||
} |
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.
Is there a similar capability-testing concern with field types that support phrase queries?
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, another solution would be to remove the selection logic entirely and rely on the fact that we force lenient mode when all fields are requested (*
).
This commit changes the query field expansion for query parsers to not rely on an hardcoded list of field types. Instead we rely on the type of exception that is thrown by MappedFieldType#termQuery to include/exclude an expanded field. Supersedes elastic#31655 Closes elastic#31798
Superseded by #33020 |
Currently this is a hardcoded whitelist, which is problematic due to the fact
that plugins can't add new types to this list, we already have the problem with
the
scaled_float
type today. This change proposes to rely on exception typesof the
termQuery
factory method instead to know whether a field should besearched.
Another option would be to add an API to MappedFieldType to check whether a
field must be considered for
all
queries, but I wanted to avoid adding a newAPI.