Add cache of query schemas to results view #3862
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
alexet
force-pushed
the
alexet/bqrs-schema-cache
branch
from
c04893c to
605712b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
extensions/ql-vscode/src/language-support/contextual/cached-operation.ts
Fixed
Show fixed
Hide fixed
extensions/ql-vscode/src/language-support/contextual/cached-operation.ts
Fixed
Show fixed
Hide fixed
alexet
force-pushed
the
alexet/bqrs-schema-cache
branch
from
605712b to
1982f52
Compare
|
I merged into main. The E2E tests should be fixed now. |
|
Hmmm....maybe not. |
|
I merged in main in another PR and got the same error: https://github.com/github/vscode-codeql/actions/runs/12282822206/job/34274881774?pr=3867 |
It is currently expected to fail due to a limitation in the E2E tests: they always install the released version, while the tests are testing the current version on the branch. See #3853. The E2E tests are not required, so they can be ignored. |
|
We need to do a new extension release in order to get the tests passing again. See #3869. |
aeisenberg
approved these changes
Dec 12, 2024
aeisenberg
reviewed
Dec 12, 2024
Comment on lines
+59
to
+63
| if (this.generation !== origGeneration) { | ||
| // Cache was reset in the meantime so don't trust this | ||
| // result enough to cache it. | ||
| return result; | ||
| } |
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
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.
The pagination this does is slow as it scan to compute the index of each page. By caching we mostly only do the scanning once and then later we can change page quickly. Unfortunately we must clear the cache on sorting though as that may reuse an existing bqrs.
For very large result sets (my test had 20,000,000 tuples) this takes the time to change page from about 10s to seemingly instant to me (I didn't precisely measure this, I just looked at a clock to get 10s) making looking through sorted data for certain results much easier.
I also removed a call to bqrs info with a page size of zero. Actually passing the pageSize of zero to the cli would result in an error, but as 0 is falsy so we treat it the same as the case without a pagesize. It seems clearer to just not provide the argument.
I am not sure why the E2E tests are failing though so I may need help debugging that.