[Discover] Improve performance of getFieldsToShow #144672
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
majagrubic
reviewed
Nov 7, 2022
kertal
added
Feature:Discover
majagrubic
reviewed
Nov 10, 2022
| if (mapped && subTypeMulti?.multi?.parent) { | ||
| childParentFieldsMap[mapped.name] = subTypeMulti.multi.parent; | ||
| childParentFieldsMap[key] = subTypeMulti.multi.parent; |
There was a problem hiding this comment.
I don't think these two are necessarily the same.
There was a problem hiding this comment.
we're getting the mapped already by key, and the mapping function uses getByName for get the field, so I assume map === key in this case?
|
@elasticmachine merge upstream |
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
majagrubic
approved these changes
Nov 15, 2022
jughosta
reviewed
Nov 15, 2022
| const parent = childParentFieldsMap[key]; | ||
| return showMultiFields || (parent && !fields.includes(parent)); | ||
| const parent = childParentFieldsMap.get(key); | ||
| return parent && !fieldSet.has(parent); |
There was a problem hiding this comment.
We could simplify even more as the following:
export const getFieldsToShow = (fields: string[], dataView: DataView, showMultiFields: boolean) => {
if (showMultiFields) {
return fields;
}
return fields.filter((fieldName) => {
const mapped = dataView.fields.getByName(fieldName);
const isMultiField = Boolean(mapped && getFieldSubtypeMulti(mapped.spec));
return !isMultiField;
});
};
It's the same way as we filter subfields from the sidebar.
There was a problem hiding this comment.
This is a great suggestion, it further simplifies the code, removes another iteration of the field list, aligns the code ... and works, applied the change
There was a problem hiding this comment.
@majagrubic this change seems to break a legacy table test
It expects city.raw to be rendered, because city is not returned by the fields API .. but city is in the mapping. I simply can't remember why we were doing this and if this is an actual case to be considered?
@jughosta To be safe we could revert the last change for this PR, apply it in a follow up for the next minor, without backporting ... getting rid of legacy
There was a problem hiding this comment.
@jughosta wanna create the follow up PR with your suggestion, just for 8.7?
There was a problem hiding this comment.
Follow up PR #145698
I kept the parent-checking logic too.
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @kertal |
kertal
force-pushed
the
discover-fix-fields-to-show-performance
branch
from
3726410 to
3ab4d6a
Compare
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Nov 16, 2022
## Summary Significantly improves the performance of `getFieldsToShow` for very large data views (>=100k). This is done by reducing the number ofiterations necessary to detect multi fields shat should not be displayed. (cherry picked from commit 1ffb93d)
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Nov 16, 2022
…45424) # Backport This will backport the following commits from `main` to `8.5`: - [[Discover] Improve performance of getFieldsToShow (#144672)](#144672) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Matthias Wilhelm","email":"matthias.wilhelm@elastic.co"},"sourceCommit":{"committedDate":"2022-11-16T17:53:18Z","message":"[Discover] Improve performance of getFieldsToShow (#144672)\n\n## Summary\r\n\r\nSignificantly improves the performance of `getFieldsToShow` for very large data views (>=100k). This is done by reducing the number ofiterations necessary to detect multi fields shat should not be displayed.","sha":"1ffb93d687e899ec93b4e1735458e9e2863d6b39","branchLabelMapping":{"^v8.6.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:fix","Team:DataDiscovery","v8.6.0","v7.17.8","v8.7.0","v8.5.2"],"number":144672,"url":"https://github.com/elastic/kibana/pull/144672","mergeCommit":{"message":"[Discover] Improve performance of getFieldsToShow (#144672)\n\n## Summary\r\n\r\nSignificantly improves the performance of `getFieldsToShow` for very large data views (>=100k). This is done by reducing the number ofiterations necessary to detect multi fields shat should not be displayed.","sha":"1ffb93d687e899ec93b4e1735458e9e2863d6b39"}},"sourceBranch":"main","suggestedTargetBranches":["7.17","8.7","8.5"],"targetPullRequestStates":[{"branch":"main","label":"v8.6.0","labelRegex":"^v8.6.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/144672","number":144672,"mergeCommit":{"message":"[Discover] Improve performance of getFieldsToShow (#144672)\n\n## Summary\r\n\r\nSignificantly improves the performance of `getFieldsToShow` for very large data views (>=100k). This is done by reducing the number ofiterations necessary to detect multi fields shat should not be displayed.","sha":"1ffb93d687e899ec93b4e1735458e9e2863d6b39"}},{"branch":"7.17","label":"v7.17.8","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.5","label":"v8.5.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>
jughosta
added a commit
that referenced
this pull request
Nov 22, 2022
…145698) A follow up for #144672 (issue #144673) ## Summary This PR reduces number of iterations through fields array by simplifying the logic and switching to `Map` and `.get()` instead of using `.includes()` on a visible fields array for each rendered field. Also `shouldShowFieldHandler` can be further improved in the future without the need to refactor how it's being used. Before: <img width="1490" alt="Screenshot 2022-11-21 at 19 36 17" src="https://user-images.githubusercontent.com/1415710/203134749-190df445-f0dc-4a66-8797-21fedc1345d2.png"> <img width="430" alt="Screenshot 2022-11-21 at 19 36 43" src="https://user-images.githubusercontent.com/1415710/203134792-06b659cd-b4e0-4248-b1b2-63767528738b.png"> <img width="406" alt="Screenshot 2022-11-21 at 19 39 05" src="https://user-images.githubusercontent.com/1415710/203134836-2881e4bf-a9be-4a12-bcd5-76074d3ae0d3.png"> After: <img width="1493" alt="Screenshot 2022-11-21 at 19 42 48" src="https://user-images.githubusercontent.com/1415710/203134864-9d686705-891f-4640-96e6-29e7af5ae9d9.png"> <img width="496" alt="Screenshot 2022-11-21 at 19 44 54" src="https://user-images.githubusercontent.com/1415710/203135114-fda20375-fd3f-4ab4-b3e5-cdf7758ca3c8.png"> <img width="370" alt="Screenshot 2022-11-21 at 19 44 32" src="https://user-images.githubusercontent.com/1415710/203135038-c4fd40b7-1c8a-43a3-b08c-c4c19c74743f.png">
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kertal
added a commit
to kertal/kibana
that referenced
this pull request
Mar 14, 2024
## Summary Significantly improves the performance of `getFieldsToShow` for very large data views (>=100k). This is done by reducing the number ofiterations necessary to detect multi fields shat should not be displayed. (cherry picked from commit 1ffb93d) # Conflicts: # src/plugins/discover/public/application/helpers/get_fields_to_show.ts
kertal
added a commit
that referenced
this pull request
Mar 18, 2024
…178731) # Backport Backport of the following commits from `main` to `7.17`: - [[Discover] Improve performance of getFieldsToShow (#144672)](#144672) ## Summary Significantly improves the performance of `getFieldsToShow` for very large data views (>=100k). This is done by reducing the number of iterations necessary to detect multi fields shat should not be displayed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR significantly improves the performance of
getFieldsToShowfor very large data views (>=100k). This is done by reducing the number of iterations necessary to detect multi fields shat should not be displayed. The number of iterations caused significant delays when rendering the document table (document explorer and classic). With a given example dataset and a mapping containing 126k fields, the performance was significantly better:Before - 34s of calculation time when rendering the table
After - 1,6 s of calculation time when rendering the table
Fixes #144673