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

[SearchBar] Use bool query instead of match bool logic #6220

Merged

Conversation

PhaedrusTheGreek
Copy link
Contributor

Summary

This replaces a single match query with bool should or must logic in Query.toESQuery(). As a match query, fields with non-text mappings were failing search due to reliance on a text analyzer which doesn't exist in that case.

closes #6217

Checklist

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6220/

@thompsongl
Copy link
Contributor

Makes sense to me. I'd like to have @chandlerprall look also as he's much more familiar with this code.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Code change LGTM, most of the snapshot changes LGTM (I've never been good at ES queries), @PhaedrusTheGreek would you mind verifying this snapshot DSL performs its intent?

exports[`astToEsQueryDsl ast - 'john group:(eng or "marketing org") -group:"kibana team" 1`] = `
Object {
"bool": Object {
"must": Array [
Object {
"simple_query_string": Object {
"query": "john",
},
},
Object {
"bool": Object {
"should": Array [
Object {
"bool": Object {
"should": Array [
Object {
"match": Object {
"group": "eng",
},
},
],
},
},
Object {
"match_phrase": Object {
"group": "marketing org",
},
},
],
},
},
],
"must_not": Array [
Object {
"match_phrase": Object {
"group": "kibana team",
},
},
],
},
}
`;

@PhaedrusTheGreek
Copy link
Contributor Author

@chandlerprall @thompsongl , the snapshot DSL does look correct, aside from the unnecessary bool on a single clause, fix for which I just pushed.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Additional changes LGTM (nice differentiating on term count!); Needs a changelog entry, the process for creating one is at https://github.com/elastic/eui/blob/main/wiki/documentation-guidelines.md#changelog - shout if something in that doc doesn't work as expected

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6220/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6220/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SearchBar] Field value selection doesn't work on keyword fields by default
4 participants