-
Notifications
You must be signed in to change notification settings - Fork 202
Add command for running a query suite #4037
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
base: main
Are you sure you want to change the base?
Conversation
bcaab92
to
a295f63
Compare
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.
Pull Request Overview
This PR introduces a new “Run Query Suite” canary feature for .qls
files and refactors local query execution to support multiple queries in one run.
- Adds
codeQL.runQuerySuite
command, context menu entries, and CLI feature guard - Refactors query-run flows to use
queryInputsOutputs
lists and per-queryoutputBaseName
- Updates
QueryOutputDir
to generate.bqrs
,.csv
, and interpreted paths dynamically
Reviewed Changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
suggestion-queries.ts | Decode BQRS with per-query basename |
model-editor-queries.ts | Read model-editor results via per-query basename |
generate.ts | Decode generate query with per-query basename |
run-query.ts | Wrap single-run in queryInputsOutputs ; lookup by path |
results-view.ts | Pass separate resultsPath and interpretedResultsPath |
query-output-dir.ts | Replace fixed paths with getBqrsPath /getCsvPath /getIntereptedResultsPath |
local-query-run.ts | Adapt history and eval-log summary to multi-query runs |
local-queries.ts | Implement runQuerySuite command and multi-query runner |
template-provider.ts | Use per-query BQRS path |
query-resolver.ts | Single-query runner updated to use queryInputsOutputs |
location-finder.ts | Fetch links from multi-query BQRS via dynamic path |
ast-builder.ts | Consume BQRS path directly |
debugger-ui.ts & debug-session.ts | Adapt debugger to per-query result structures |
debug-protocol.ts | Add outputBaseName to protocol |
interpreted-results.ts & compare-view.ts | Reference new resultsPath properties |
common/commands.ts | Register codeQL.runQuerySuite command |
cli-version.ts | Expose queryServerRunQueries feature flag |
package.json | Add command and menu entries for runQuerySuite |
return join(this.querySaveDir, `${outputBaseName}.bqrs`); | ||
} | ||
|
||
getIntereptedResultsPath( |
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.
The method name getIntereptedResultsPath is misspelled; it should be getInterpretedResultsPath.
getIntereptedResultsPath( | |
getInterpretedResultsPath( |
Copilot uses AI. Check for mistakes.
const queryResults = Array.from(completedQuery.results.values()); | ||
if (queryResults.length !== 1) { | ||
throw new Error( | ||
`Expected exactly one query result, but got ${queryResults.length}`, | ||
); | ||
} | ||
const bqrs = await cliServer.bqrsDecodeAll( | ||
completedQuery.outputDir.getBqrsPath(queryResults[0].outputBaseName), |
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.
[nitpick] This pattern for extracting and validating a single query result is repeated in multiple locations; consider extracting it into a shared helper to reduce duplication.
const queryResults = Array.from(completedQuery.results.values()); | |
if (queryResults.length !== 1) { | |
throw new Error( | |
`Expected exactly one query result, but got ${queryResults.length}`, | |
); | |
} | |
const bqrs = await cliServer.bqrsDecodeAll( | |
completedQuery.outputDir.getBqrsPath(queryResults[0].outputBaseName), | |
const bqrs = await extractAndDecodeSingleQueryResult( | |
completedQuery.results, | |
cliServer, | |
completedQuery.outputDir, |
Copilot uses AI. Check for mistakes.
@@ -34,7 +34,7 @@ export async function runQuery({ | |||
const queryRun = queryRunner.createQueryRun( | |||
databaseItem.databaseUri.fsPath, | |||
{ | |||
queryPath, | |||
queryInputsOutputs: [{ queryPath, outputBaseName: "results" }], |
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.
[nitpick] Hard-coding the outputBaseName string "results" in multiple places could be error-prone; consider defining a constant for the default basename.
queryInputsOutputs: [{ queryPath, outputBaseName: "results" }], | |
queryInputsOutputs: [{ queryPath, outputBaseName: DEFAULT_OUTPUT_BASENAME }], |
Copilot uses AI. Check for mistakes.
This uses a new query-server command for running multiple queries, so that a single evaluator log will be produced for the entire run. To avoid too much code duplication, I have updated a lot of the code paths involved in running local queries to work with multiple query paths. This also required some refactoring to explicitly associate an output basename (used to produce the .bqrs, .csv, etc. paths) with each query, where before those output filenames were hard-coded.
a295f63
to
2f44cb0
Compare
The command is visible in the context menu for a
.qls
file in the Explorer pane, only when canary mode is enabled.This uses a new query-server command for running multiple queries, so that a single evaluator log will be produced for the entire run. The intended use case is an internal QL author who wants to judge performance by viewing the evaluator log for the entire query suite.
It would have been much simpler to use the existing functionality for running multiple queries, but that functionality does not work very well when one wants to inspect the evaluator logs – since each query gets started in turn, which truncates the log for the previous query and starts a new one.
To avoid too much code duplication, I have updated a lot of the code paths involved in running local queries to work with multiple query paths. This also required some refactoring to explicitly associate an output basename (used to produce the
.bqrs
,.csv
, etc. paths) with each input query, where before those output filenames were hard-coded. As a result, the diff is fairly big (but the changes should be easily understandable).Since this is a canary feature intended for internal users, and in the interest of shipping it quickly, there are a few rough edges, particularly with the UI for the query history pane:
In the long term, it would be ideal to have a first-class concept of a multi-query run in the query history pane, which would allow us to create hierarchical entries, where run-specific commands like "Show evaluator log summary" would appear on the root node for the run, while query-specific commands like "Show CSV" would appear on the child nodes for each query. But I think what's implemented here is good enough for a canary/internal feature.
Also, if the CLI is too old to support the new
evaluation/runQueries
query-server command, the run will fail with an error message explaining this. I decided not to implement a fallback to running the queries one by one.This is a canary feature, so I won't update the changelog.