rusackas commented on PR #40039: URL: https://github.com/apache/superset/pull/40039#issuecomment-4512693294
Concern: sortComparatorForNoSearch in the accumulation. The accumulated base options are sorted on every page arrival. For large datasets this is O(n log n) on each page, where n grows. This was not in the original code and could become noticeably slow for very large cached option sets. Consider accumulating unsorted and sorting only once when restoring. Concern: sort(sortComparatorForNoSearch) on search results. The server already returned results ranked by relevance. Sorting client-side with sortComparatorForNoSearch could reorder them and push the most relevant match down. Is this intentional? The existing mergeData also sorts, but for search results, server ranking is typically preferred. This deserves a comment or consideration. Concern: page > 0 during search (the else branch, line 401+). The "append normally" path for search pagination pages isn't explicitly shown in the diff but falls through to the original mergeData path. Verify that paginating through search results (scrolling the dropdown) still works correctly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
