Skip to content
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

Undo changes made in PR #2218 and optimize copying query results #2385

Merged
merged 8 commits into from
Sep 12, 2024

Conversation

lewis-sanchez
Copy link
Contributor

@lewis-sanchez lewis-sanchez commented Aug 22, 2024

This PR is a part of microsoft/azuredatastudio#25632

This PR undoes #2218, which removed functionality to copy query results from SQL Tools Service (STS). This PR brings back functionality to copy results into the clipboard from STS. This PR also removes a bottleneck that was making it take a long time to copy query results because the ToString method was being repeatedly invoked on the string builder that is assembling the query results string that is added to the clipboard.

After optimizing the copy endpoint to stop repeatedly invoking ToString() on the StringBuilder the time needed to copy query results became significantly faster:

Before removing the repeated ToString() calls:
Copying 25,000 rows took 7,982 ms
Copying 50,000 rows took 34,863 ms
Copying 75,000 rows took 76,430 ms
Copying 100,000 rows took 163,462 ms

After removing repeated ToString() calls:
Copying 25,000 rows took 1.0278617 ms
Copying 50,000 rows took 1.9388735 ms
Copying 75,000 rows took 2.8113253 ms
Copying 100,000 rows took 3.8120204 ms

Copy link

As part of updating the dependencies in Packages.props we require that any PRs opened also verify that
they've done the following checks.

Please respond to this comment verifying that you've done the appropriate validation (or explain why it's not necessary) before merging in the PR

  • Built and tested the change locally to validate that the update doesn't cause any regressions and fixes the issues intended
  • Tested changes on all major platforms
    • Windows
    • Linux
    • Mac
  • Check the source repo for any open issues with the release being updated to (if available)

Packages.props Show resolved Hide resolved
@kburtram
Copy link
Member

@lewis-sanchez is it possible to do some basic benchmarking for this change? For example, with some very large resultssets how long does copy take with & without this change on various environments (specifically Linux and Windows). With all the previous PRs in this area linked above, I want to make sure we're address the issue reported in the most recent bug.

@lewis-sanchez
Copy link
Contributor Author

@lewis-sanchez is it possible to do some basic benchmarking for this change? For example, with some very large resultssets how long does copy take with & without this change on various environments (specifically Linux and Windows). With all the previous PRs in this area linked above, I want to make sure we're address the issue reported in the most recent bug.

@kburtram, yes, it should be possible to do some benchmarking for this change.

@lewis-sanchez lewis-sanchez marked this pull request as draft August 28, 2024 01:09
@lewis-sanchez lewis-sanchez marked this pull request as ready for review August 29, 2024 02:14
@lewis-sanchez lewis-sanchez changed the title Copy query results to the clipboard from STS. Undo changes made in PR #2218 Aug 29, 2024
@lewis-sanchez lewis-sanchez changed the title Undo changes made in PR #2218 Undo changes made in PR #2218 and optimize copying query results Sep 4, 2024
@lewis-sanchez lewis-sanchez merged commit 4646cd3 into main Sep 12, 2024
3 checks passed
@lewis-sanchez lewis-sanchez deleted the lewissanchez/bug/copy-results-to-clipboard branch September 12, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants