-
Notifications
You must be signed in to change notification settings - Fork 7
Issue 53997: Establish a maximum size for query selections #7107
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
Conversation
labkey-jeckels
left a comment
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.
Maybe I'm misreading the code or expected behavior, but it seems like this won't be true if there are any existing rows selected that aren't in the first M rows:
The query-selectAll.api will now select up to the first M rows
I think createSelectionSet() will return 100,000 entries, which will then be rejected when merged with the existing selection in setSelected().
Maybe that's fine, but it's a more muddled of a behavior to describe.
74eee43 to
21c4cde
Compare
e496054 to
2307412
Compare
| values = StringUtils.split(values[0],'\t'); | ||
| List<String> parameterSelected = values == null ? new ArrayList<>() : Arrays.asList(values); | ||
| Set<String> result = new LinkedHashSet<>(parameterSelected); | ||
| Set<String> result = values == null ? new LinkedHashSet<>() : new LinkedHashSet<>(Arrays.asList(values)); |
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.
LinkedHashSet uses significantly more memory than ArrayList.
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.
Understood but the ArrayList here was just interstitial. Mainly looking to go from a String[] to a LinkedHashSet as quickly as possible.
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.
In general, the change here to stop converting to lists is significantly more performant since we end up doing set logic on these and conversions between the same types is much faster. Saw a significant reduction in processing time switching everything to using the same set structure (e.g., "Clear all" selection).
1be74a6 to
4715dd7
Compare
Rationale
This addresses Issue 53997 by establishing a maximum query selection size of (M=100,000). Some things to note:
API Changes
query-selectAll.apiwill now select up to the first M rows. If the underlying query contains more rows than the maximum, then the first M will be selected. If the underlying query contains less than M rows, then all rows will be selected.query-setSelection.api(namedQueryController.SetCheckAction) will reject any selection that will result in a selection size over the maximum. When rejected the pre-existing selection will remain unchanged.UI Changes
Related Pull Requests
Changes
LinkedHashSetmaking comparison/iteration operations much faster. Formerly, we were converting between different structures which resulted in a lot more time composing/iterating (e.g., clearing a selection where the selection size is very large).Select First M Rowsbutton when applicable.