-
Notifications
You must be signed in to change notification settings - Fork 842
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
[SearchBar] Use bool query instead of match bool logic #6220
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6220/ |
Makes sense to me. I'd like to have @chandlerprall look also as he's much more familiar with this code. |
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.
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?
eui/src/components/search_bar/query/__snapshots__/ast_to_es_query_dsl.test.ts.snap
Lines 161 to 202 in b4e538d
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", | |
}, | |
}, | |
], | |
}, | |
} | |
`; |
src/components/search_bar/query/__snapshots__/ast_to_es_query_dsl.test.ts.snap
Outdated
Show resolved
Hide resolved
@chandlerprall @thompsongl , the snapshot DSL does look correct, aside from the unnecessary bool on a single clause, fix for which I just pushed. |
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.
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
Preview documentation changes for this PR: https://eui.elastic.co/pr_6220/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6220/ |
Summary
This replaces a single match query with bool
should
ormust
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