Change the way that fields are selected for inclusion in all queries.#31655
Closed
jpountz wants to merge 1 commit intoelastic:masterfrom
Closed
Change the way that fields are selected for inclusion in all queries.#31655jpountz wants to merge 1 commit intoelastic:masterfrom
all queries.#31655jpountz wants to merge 1 commit intoelastic:masterfrom
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.
Collaborator
|
Pinging @elastic/es-search-aggs |
markharwood
reviewed
Jun 29, 2018
| * @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 | ||
| */ |
Contributor
There was a problem hiding this comment.
The last 2 javadoc "throws" descriptions give identical reasons?
markharwood
reviewed
Jun 29, 2018
| } | ||
| if (acceptMetadataField == false && mapper instanceof MetadataFieldMapper) { | ||
| if (acceptMetadataField == false && ft.name().startsWith("_")) { | ||
| // Ignore metadata fields |
Contributor
There was a problem hiding this comment.
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?)
markharwood
reviewed
Jun 29, 2018
| continue; | ||
| } catch (RuntimeException e) { | ||
| // other exceptions are parsing errors or not indexed fields: keep | ||
| } |
Contributor
There was a problem hiding this comment.
Is there a similar capability-testing concern with field types that support phrase queries?
jimczi
approved these changes
Jul 6, 2018
Contributor
jimczi
left a comment
There was a problem hiding this comment.
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 (*).
jimczi
added a commit
to jimczi/elasticsearch
that referenced
this pull request
Aug 21, 2018
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
Contributor
|
Superseded by #33020 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_floattype today. This change proposes to rely on exception typesof the
termQueryfactory 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
allqueries, but I wanted to avoid adding a newAPI.