-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Fix timefield sorting when switching similar index patterns #116145
Conversation
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
src/plugins/discover/public/application/apps/main/utils/get_switch_index_pattern_app_state.ts
Outdated
Show resolved
Hide resolved
…imefield-sorting
src/plugins/discover/public/application/main/utils/get_switch_index_pattern_app_state.ts
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/utils/get_switch_index_pattern_app_state.ts
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/utils/get_switch_index_pattern_app_state.ts
Outdated
Show resolved
Hide resolved
@@ -290,11 +298,11 @@ export class DataView implements IIndexPattern { | |||
return [...this.fields.getAll().filter((field) => field.scripted)]; | |||
} | |||
|
|||
isTimeBased(): boolean { | |||
isTimeBased(): this is TimeBasedDataView { |
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.
ℹ️ This way we get better TS support. Calling that method now, will cause the compiler to understand, that if that returns true
timeFieldName
must be a string
and can't be undefined
anymore.
@elasticmachine merge upstream |
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.
Thanks, nice improvement! code lgtm!
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.
Thx for all those changes! ❤️ So I did some 🥣 a-la-carte
testing
- I've installed our ecommerce sample data
kibana_sample_data_ecommerce
(timeFieldorder_date
) - I created a new data view
*ecommerce*
(timeField:products.created_on
) - I've selected
kibana_sample_data_ecommerce
in Discover, no changes - When I switched from
kibana_sample_data_ecommerce
(no sort change) to*ecommerce*
, the documents were now sorted byproducts.created_on
... this is how it should behave 👍
Before it was still sorted by order_date
- Now I switched to
kibana_sample_data_ecommerce
, - Sorted by
products.product_name.keyword,order_date
, so it's a non default sort with the timefield on the second place. - Switched to
*ecommerce*
- The result was, that the sorting was kept, which I expected to be fine, but since there's a different time column configured, there's no more way to remove the sorting unless changing the URL:
So I think we need to consider also this case, but I'm not 100% sure how.
There are 2 ways - We remove the timefield of the previous data field in this case (🚀)
- We replace it with the timefield the user has just selected (❤️ )
Adding another 2 pair of eyes here @majagrubic @timroes
@elasticmachine merge upstream |
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.
added 2 hopefully last comments. thx so much 🙏 for your patience
src/plugins/discover/public/application/main/utils/get_switch_index_pattern_app_state.ts
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/utils/get_switch_index_pattern_app_state.ts
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: cc @dmitriynj |
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.
LGTM, thx so much! 👍
Final test by using this data (one of the examples of the issues linked)
Steps to reproduce:
Create 2 indices, and 2 index patterns: timefield
is the time field of my_index
and timefield_2
is the time field of my_index_2
. In my_index_2
also add timefield
to the mapping but it won't contain data:
PUT my_index
{
"mappings": {
"properties": {
"timefield": {
"type": "date"
}
}
}
}
POST my_index/_bulk
{ "index": {} }
{ "timefield": "2021-08-05" }
{ "index": {} }
{ "timefield": "2021-08-04" }
{ "index": {} }
{ "timefield": "2021-08-03" }
PUT my_index_2
{
"mappings": {
"properties": {
"timefield": {
"type": "date"
},
"timefield_2": {
"type": "date"
}
}
}
}
POST my_index_2/_bulk
{ "index": {} }
{ "timefield_2": "2021-08-05" }
{ "index": {} }
{ "timefield_2": "2021-08-04" }
{ "index": {} }
{ "timefield_2": "2021-08-03" }
Now in Discover, when switching from my_index
to my_index_2
, Discover sees the current sorting field (timefield
) exists in my_index_2
and doesn’t change it. Since there is no data in this field in my_index_2
, it shows up as unsorted.
With this PR
It's fixed 🎂 , switching the to my_index_2
also switches the timeField, so it works
elastic#116145) * [Discover] fix timefield sorting * [Discover] resolve review comments * [Discover] replace primary time field for sorting * [Discover] improve solution * Improve typing and time based checking * Fix missing isTimeBased call * [Discover] fix linting * [Discover] replace time fields when switching between time based data views * [Discover] apply suggestions Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Tim Roes <[email protected]>
💔 Backport failed
Successful backport PRs will be merged automatically after passing CI. To backport manually run: |
#116145) (#119492) * [Discover] fix timefield sorting * [Discover] resolve review comments * [Discover] replace primary time field for sorting * [Discover] improve solution * Improve typing and time based checking * Fix missing isTimeBased call * [Discover] fix linting * [Discover] replace time fields when switching between time based data views * [Discover] apply suggestions Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Tim Roes <[email protected]> Co-authored-by: Dmitry Tomashevich <[email protected]> Co-authored-by: Tim Roes <[email protected]>
#116145) * [Discover] fix timefield sorting * [Discover] resolve review comments * [Discover] replace primary time field for sorting * [Discover] improve solution * Improve typing and time based checking * Fix missing isTimeBased call * [Discover] fix linting * [Discover] replace time fields when switching between time based data views * [Discover] apply suggestions Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Tim Roes <[email protected]>
elastic#116145) * [Discover] fix timefield sorting * [Discover] resolve review comments * [Discover] replace primary time field for sorting * [Discover] improve solution * Improve typing and time based checking * Fix missing isTimeBased call * [Discover] fix linting * [Discover] replace time fields when switching between time based data views * [Discover] apply suggestions Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Tim Roes <[email protected]>
Summary
Fixes #108186
Fixes #117617
Sorting should be set properly after changing index patterns with the same field names, but different timefields. Testing can be done using data provided in the issue.
Checklist