[AMORO-2215] Optimize the speed of searching for tables in the tables navigation bar #2452
Conversation
|
@zhoujinsong @hameizi Can you take some time to review the PR? I have tested in my own env, in which it works ok. |
|
Sorry for the late reply. @tcodehuber We have recently done some refactoring on the front-end, so there are some conflicts that need to be resolved. Afterward, we can proceed with merging this PR. BTW, there are some changes when building the front-end, you can refer to |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2452 +/- ##
============================================
+ Coverage 33.96% 34.71% +0.74%
- Complexity 4357 4521 +164
============================================
Files 604 608 +4
Lines 50754 50980 +226
Branches 6673 6686 +13
============================================
+ Hits 17241 17698 +457
+ Misses 32124 31829 -295
- Partials 1389 1453 +64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@wangtaohz I have resolved conflicts, please review it again. |
wangtaohz
changed the title
| if (state.allDatabaseListLoaded.length) { | ||
| state.databaseList = state.allDatabaseListLoaded.filter((ele) => { | ||
| return ele.label.includes(state.DBSearchInput) | ||
| }) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe can use computed to implement this filtering instead of creating a new state
There was a problem hiding this comment.
But I have to store all databases/tables in the frontend, then filtering by keyword can be acted at them. the databaseList and tableLIst in the state just are just used as values displaying in virtual-recycle-scroller. Or can i move the allDatabaseListLoaded/allTableListLoaded out of const state = reactive({
There was a problem hiding this comment.
There is no conflict between the two. You just need to remove database and table and filter with computed.
There was a problem hiding this comment.
@zhangmo8 Can you review it again?
| @@ -283,10 +305,14 @@ export default defineComponent({ | |||
| label: ele.name, | |||
| type: ele.type | |||
| })) | |||
| if (state.tableSearchInput === '') { | |||
There was a problem hiding this comment.
I suggest moving the filtering of databases and tables from the backend to the frontend and the HTTP API always returns all databases and all tables. Then there will be no need for additional checks on whether allTableListLoaded should be set.
BTW, removing the filtering from the HTTP API won't incur additional overhead on the backend, because the filtering takes effect at the final step of returning results.
There was a problem hiding this comment.
Yes, filtering of databases and tables is acted in the frontend now. We only need load all databases or tables for the first time. Afterwards, keyword filtering will not request HTTP API.
| @@ -271,6 +286,13 @@ export default defineComponent({ | |||
| if (!state.curCatalog || !state.database) { | |||
| return | |||
| } | |||
| if (state.allTableListLoaded.length) { | |||
There was a problem hiding this comment.
I'm not sure if the tableList here is still old after switching database, if yes, maybe you need to save a mapping from database to tableList?
There was a problem hiding this comment.
No, in the const handleClickDb = (item: IDatabaseItem) function, i have set state.allTableListLoaded.length = 0.
There was a problem hiding this comment.
Thanks a lot for your contribution! @tcodehuber
Thanks a lot for your reviewing! @zhangmo8
Why are the changes needed?
Close #2215.
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation