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

Allow fields to be set to * #42301

Merged
merged 4 commits into from
May 23, 2019
Merged

Allow fields to be set to * #42301

merged 4 commits into from
May 23, 2019

Conversation

matriv
Copy link
Contributor

@matriv matriv commented May 21, 2019

Allow for SimpleQueryString, QueryString and MultiMatchQuery
to set the fields parameter to the wildcard *. If so, set
the leniency to true, to achieve the same behaviour as from the
"default_field" : "*" setting.

Closes: #39577

Allow for SimpleQueryString, QueryString and MultiMatchQuery
to set the `fields` parameter to the wildcard `*`. If so, set
the leniency to `true`, to achieve the same behaviour as from the
`"default_field" : "*" setting.

Closes: elastic#39577
@matriv matriv added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.3.0 labels May 21, 2019
@matriv matriv requested a review from jimczi May 21, 2019 15:42
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

if (isAllField) {
newSettings.lenient(lenientSet ? settings.lenient() : true);
}
isAllField = defaultFields.size() == 1 && Regex.isMatchAllPattern(defaultFields.get(0));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jimczi Why do we check only the first element here, shouldn't we check all the defaultFields?
(same applies to all 3 types of queries)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should check all elements. However I don't think it's useful to set fields or the default fields to something like ["*", "field1", "field2"] but for consistency we should always set the leniency to true if * is part of the list of fields to query.

@matriv matriv requested a review from cbuescher May 21, 2019 16:27
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left some small comments but it looks good @matriv.

if (isAllField) {
newSettings.lenient(lenientSet ? settings.lenient() : true);
}
isAllField = defaultFields.size() == 1 && Regex.isMatchAllPattern(defaultFields.get(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should check all elements. However I don't think it's useful to set fields or the default fields to something like ["*", "field1", "field2"] but for consistency we should always set the leniency to true if * is part of the list of fields to query.

}
assertEquals(11, noMatchNoDocsQueries);
assertThat(disjunctionMaxQuery.getDisjuncts(), hasItems(new TermQuery(new Term(STRING_FIELD_NAME, "hello")),
new TermQuery(new Term(STRING_FIELD_NAME_2, "hello"))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a test with a single field set on * ?

}
assertEquals(11, noMatchNoDocsQueries);
assertThat(disjunctionMaxQuery.getDisjuncts(), hasItems(new TermQuery(new Term(STRING_FIELD_NAME, "hello")),
new TermQuery(new Term(STRING_FIELD_NAME_2, "hello"))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

matriv and others added 2 commits May 22, 2019 12:11
Also check if `*` is in the list of the default_field but
not necessarily as the 1st element.
@matriv matriv requested a review from jimczi May 22, 2019 10:13
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

The main part looks good to me, I left a very small suggestion about testing.

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM too

@matriv matriv merged commit e75ff0c into elastic:master May 23, 2019
@matriv matriv deleted the fix-39577 branch May 23, 2019 08:10
matriv added a commit that referenced this pull request May 23, 2019
Allow for SimpleQueryString, QueryString and MultiMatchQuery
to set the `fields` parameter to the wildcard `*`. If so, set
the leniency to `true`, to achieve the same behaviour as from the
`"default_field" : "*" setting.

Furthermore,  check if `*` is in the list of the `default_field` but
not necessarily as the 1st element.

Closes: #39577
(cherry picked from commit e75ff0c)
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
Allow for SimpleQueryString, QueryString and MultiMatchQuery
to set the `fields` parameter to the wildcard `*`. If so, set
the leniency to `true`, to achieve the same behaviour as from the
`"default_field" : "*" setting.

Furthermore,  check if `*` is in the list of the `default_field` but
not necessarily as the 1st element.

Closes: elastic#39577
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v7.3.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

simple_query_string with fields: ["*"] can result in an error
5 participants