[Cases] - Use text field for phrase prefix queries#235290
[Cases] - Use text field for phrase prefix queries#235290michaelolo24 merged 10 commits intoelastic:mainfrom
Conversation
|
Cloud deployment initiated, see credentials at: https://buildkite.com/elastic/kibana-deploy-cloud-from-pr/builds/421 |
|
Project deployed, see credentials at: https://buildkite.com/elastic/kibana-deploy-project-from-pr/builds/595 |
janmonschke
left a comment
There was a problem hiding this comment.
Nice catch, glad you caught it now!
6adb189 to
d68e178
Compare
|
Pinging @elastic/kibana-cases (Feature:Cases) |
| schema.object({ | ||
| keyword: schema.maybe(schema.string()), | ||
| text: schema.maybe(schema.string()), | ||
| }), |
There was a problem hiding this comment.
I don't think that we can store these. ES multifields shouldn't be part of the document.
There was a problem hiding this comment.
keyword was added in the ./v4. Do you want me to remove this field?
There was a problem hiding this comment.
Your document attributes will never have this shape:
{
...
incremental_id: { keyword: '123' }
}
Only ever the shape you write them in:
{
...
incremental_id: 123
}
The multifield definition is an instruction to ES to index the same field in different ways, not to alter the shape of document attributes (a.k.a the source). So in both v4 and v5 your forward compatibility schema should be:
incremental_id: schema.maybe(schema.nullable(schema.number()))I think this is a safe change to make retroactively to v4 as well since it's compatible with your document shape, although if your fwc schema is invoked on the next release it will allow for incremental_id: { keyword: string } | number which is not what your code expects I think. At least based on CasePersistedAttributes.
Looks like x-pack/platform/plugins/shared/cases/server/saved_object_types/cases/schemas/latest.ts still needs to be updated to point to v5.
There was a problem hiding this comment.
Thanks @jloleysens , makes sense. I've made the udpates and will be testing locally and on serverless with the associated PR!
There was a problem hiding this comment.
Left a comment about FWC schema expanding on what @afharo was saying that we should address.
Would you mind providing a simple example of the queries you are making? Just a set of requests to ES to repro what you are seeing and how this fixes it?
lenient
Core has prioritised the following work and we aim to deliver in the next month or so. It would give you far more control of the queries on your SOs so it might be worth waiting for. Although the fix to the FWC should still be done.
| schema.object({ | ||
| keyword: schema.maybe(schema.string()), | ||
| text: schema.maybe(schema.string()), | ||
| }), |
There was a problem hiding this comment.
Your document attributes will never have this shape:
{
...
incremental_id: { keyword: '123' }
}
Only ever the shape you write them in:
{
...
incremental_id: 123
}
The multifield definition is an instruction to ES to index the same field in different ways, not to alter the shape of document attributes (a.k.a the source). So in both v4 and v5 your forward compatibility schema should be:
incremental_id: schema.maybe(schema.nullable(schema.number()))I think this is a safe change to make retroactively to v4 as well since it's compatible with your document shape, although if your fwc schema is invoked on the next release it will allow for incremental_id: { keyword: string } | number which is not what your code expects I think. At least based on CasePersistedAttributes.
Looks like x-pack/platform/plugins/shared/cases/server/saved_object_types/cases/schemas/latest.ts still needs to be updated to point to v5.
jloleysens
left a comment
There was a problem hiding this comment.
I think this looks good. Thanks for addressing the feedback @michaelolo24 , I tried to suggest a couple of ways we might simplify some things!
x-pack/platform/plugins/shared/cases/server/saved_object_types/cases/schemas/v5.ts
Outdated
Show resolved
Hide resolved
x-pack/platform/plugins/shared/cases/server/saved_object_types/cases/schemas/v4.ts
Outdated
Show resolved
Hide resolved
x-pack/platform/plugins/shared/cases/server/saved_object_types/cases/versioning.test.ts
Outdated
Show resolved
Hide resolved
… src/core/server/integration_tests/ci_checks'
@jloleysens This particular prefix+wildcard query failed in the previous implementation: It seems that this combination can't be done for fields mapped as |
💔 Build Failed
Failed CI StepsMetrics [docs]
History
|
### TLDR We need to utilize `text` in place of `keyword` to not break phrase prefix queries, so introducing the field `incremental_id.text` ### Background A new issue appeared where the current search field `title` and `description` are of mapping type text. This typically shouldn't be a problem, but unfortunately when attempting to run a prefixed query with the new `incremental_id.keyword` field in [this pr](elastic#230278) we received the following error: To test, run [this test](https://github.com/elastic/kibana/blob/8ffa408f560b59cde8f045ff440090a05bf7bdbf/x-pack/platform/test/functional_with_es_ssl/apps/cases/group2/list_view.ts#L397) on [this PR](elastic#230278) (after changing all instances in the UI from `incremental_id.text` back to `incremental_id.keyword`: ``` search_phase_execution_exception Root causes: query_shard_exception: failed to create query: Can only use phrase prefix queries on text fields - not on [cases.incremental_id.keyword] which is of type [keyword] ``` After some digging as to exactly why this was happening, the only documentation regarding this we could find was in this comment: elastic#129424 (comment) . We should most likely add this information to the documentation here as well: https://www.elastic.co/docs/reference/query-languages/query-dsl/query-dsl-prefix-query ? For historical reference of mapping changes to the `incremental_id` see: elastic#234054 The primary concern for this change is initializing a mapping change in the serverless environment, but this has been tested in this pr and tested against [this PR](elastic#230278) that also enables the functionality that will populate this field. CI has also been run to make sure all tests pass with the new mapping to avoid any additional surprises [here](elastic#230278 (comment)) For testing: see elastic#230278 (comment) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
### TLDR We need to utilize `text` in place of `keyword` to not break phrase prefix queries, so introducing the field `incremental_id.text` ### Background A new issue appeared where the current search field `title` and `description` are of mapping type text. This typically shouldn't be a problem, but unfortunately when attempting to run a prefixed query with the new `incremental_id.keyword` field in [this pr](#230278) we received the following error: To test, run [this test](https://github.com/elastic/kibana/blob/8ffa408f560b59cde8f045ff440090a05bf7bdbf/x-pack/platform/test/functional_with_es_ssl/apps/cases/group2/list_view.ts#L397) on [this PR](#230278) (after changing all instances in the UI from `incremental_id.text` back to `incremental_id.keyword`: ``` search_phase_execution_exception Root causes: query_shard_exception: failed to create query: Can only use phrase prefix queries on text fields - not on [cases.incremental_id.keyword] which is of type [keyword] ``` After some digging as to exactly why this was happening, the only documentation regarding this we could find was in this comment: #129424 (comment) . We should most likely add this information to the documentation here as well: https://www.elastic.co/docs/reference/query-languages/query-dsl/query-dsl-prefix-query ? For historical reference of mapping changes to the `incremental_id` see: #234054 The primary concern for this change is initializing a mapping change in the serverless environment, but this has been tested in this pr and tested against [this PR](#230278) that also enables the functionality that will populate this field. CI has also been run to make sure all tests pass with the new mapping to avoid any additional surprises [here](#230278 (comment)) For testing: see #230278 (comment) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
### TLDR We need to utilize `text` in place of `keyword` to not break phrase prefix queries, so introducing the field `incremental_id.text` ### Background A new issue appeared where the current search field `title` and `description` are of mapping type text. This typically shouldn't be a problem, but unfortunately when attempting to run a prefixed query with the new `incremental_id.keyword` field in [this pr](elastic#230278) we received the following error: To test, run [this test](https://github.com/elastic/kibana/blob/4da9288e4d3f13e24e991f91cc51c3ef9fcf5bf7/x-pack/platform/test/functional_with_es_ssl/apps/cases/group2/list_view.ts#L397) on [this PR](elastic#230278) (after changing all instances in the UI from `incremental_id.text` back to `incremental_id.keyword`: ``` search_phase_execution_exception Root causes: query_shard_exception: failed to create query: Can only use phrase prefix queries on text fields - not on [cases.incremental_id.keyword] which is of type [keyword] ``` After some digging as to exactly why this was happening, the only documentation regarding this we could find was in this comment: elastic#129424 (comment) . We should most likely add this information to the documentation here as well: https://www.elastic.co/docs/reference/query-languages/query-dsl/query-dsl-prefix-query ? For historical reference of mapping changes to the `incremental_id` see: elastic#234054 The primary concern for this change is initializing a mapping change in the serverless environment, but this has been tested in this pr and tested against [this PR](elastic#230278) that also enables the functionality that will populate this field. CI has also been run to make sure all tests pass with the new mapping to avoid any additional surprises [here](elastic#230278 (comment)) For testing: see elastic#230278 (comment) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
TLDR
We need to utilize
textin place ofkeywordto not break phrase prefix queries, so introducing the fieldincremental_id.textBackground
A new issue appeared where the current search field
titleanddescriptionare of mapping type text. This typically shouldn't be a problem, but unfortunately when attempting to run a prefixed query with the newincremental_id.keywordfield in this pr we received the following error:To test, run this test on this PR (after changing all instances in the UI from
incremental_id.textback toincremental_id.keyword:After some digging as to exactly why this was happening, the only documentation regarding this we could find was in this comment: #129424 (comment) . We should most likely add this information to the documentation here as well: https://www.elastic.co/docs/reference/query-languages/query-dsl/query-dsl-prefix-query ?
For historical reference of mapping changes to the
incremental_idsee: #234054The primary concern for this change is initializing a mapping change in the serverless environment, but this has been tested in this pr and tested against this PR that also enables the functionality that will populate this field. CI has also been run to make sure all tests pass with the new mapping to avoid any additional surprises here
For testing: see #230278 (comment)