Skip to content
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 query_string_query to transform "foo:*" in an exists query on the field name #23433

Merged
merged 2 commits into from Mar 2, 2017
Merged

Fix query_string_query to transform "foo:*" in an exists query on the field name #23433

merged 2 commits into from Mar 2, 2017

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Mar 1, 2017

Currently "foo:*" is parsed as prefix query on the field foo unless the field is defined in default_field or fields.
This commit fixes this behavior, "foo:*" is now rewritten to an exists query on the field name.
This change also removes the assumption that "_all:*" should return all docs.

relates #23356

… field name

Currently "foo:*" is parsed as prefix query on the field `foo` unless the field is defined in `default_field` or `fields`.
This commit fixes this behavior, "foo:*" is now rewritten to an exists query on the field name.
This change also removes the assumption that "_all:*" should return all docs.

relates #23356
@jimczi jimczi added :Search/Search Search-related issues that do not fall into other categories >bug v5.4.0 v6.0.0-alpha1 labels Mar 1, 2017
@jimczi jimczi requested a review from dakrone March 1, 2017 14:35
@jpountz
Copy link
Contributor

jpountz commented Mar 1, 2017

I like the idea! Does it also work if _field_names is disabled or should we only enable this if _field_names is enabled?

@jimczi
Copy link
Contributor Author

jimczi commented Mar 1, 2017

Does it also work if _field_names is disabled or should we only enable this if _field_names is enabled?

I pushed 86d0697 to handle this case

(FieldNamesFieldMapper.FieldNamesFieldType) context.getMapperService().fullName(FieldNamesFieldMapper.NAME);
if (fieldNamesFieldType.isEnabled() == false) {
// The field_names_field is disabled so we switch to a wildcard query that matches all terms
return new WildcardQuery(new Term(queryText, "*"));
Copy link
Contributor

@jpountz jpountz Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that will only work on text/keyword fields? I think we need to perform this check up-front in the MappedFieldType?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True but that's also the current behavior. foo:* does not work if foo is not a text/keyword field. I don't get what you mean by check up-front ? Should we add another function in the MappedFieldType that returns match all query for the field ? Numbers would return open ranges query, text/keyword wildcard query and we would use this function only if the _field_names field is disabled ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sorry I thought we already had such a method, hence my comment! I think things are good this way, wildcard queries only make sense on text/keyword anyway. Thanks for the explanation!

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jimczi jimczi merged commit 6519e12 into elastic:master Mar 2, 2017
@jimczi jimczi deleted the query_string_query_prefix branch March 2, 2017 15:26
@jimczi
Copy link
Contributor Author

jimczi commented Mar 2, 2017

Thanks for reviewing @jpountz

jimczi added a commit that referenced this pull request Mar 2, 2017
… field name (#23433)

Currently "foo:*" is parsed as prefix query on the field `foo` unless the field is defined in `default_field` or `fields`.
This commit fixes this behavior, "foo:*" is now rewritten to an exists query on the field name.
This change also removes the assumption that "_all:*" should return all docs.

relates #23356
jimczi added a commit that referenced this pull request Apr 7, 2017
…all query (#23967)

This change restores the rewrite to a match all query that we used to apply on wildcard query * on the query_string parser before #23433.
jimczi added a commit that referenced this pull request Apr 7, 2017
…all query (#23967)

This change restores the rewrite to a match all query that we used to apply on wildcard query * on the query_string parser before #23433.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v5.4.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants