Added Query Plan to results pane #18124

laurennat · 2024-10-01T22:33:16Z

…cleaning up

…clean up code

github-actions · 2024-10-01T22:35:48Z

VSIX Size Comparison

Main branch VSIX size: 13192 KB
PR branch VSIX size: 13196 KB
Size difference: $${\color{green} 4 KB \space (0\%) }$$

React Webview Bundle Size Comparison

Main branch bundle size: 2944 KB
PR branch bundle size: 2956 KB
Size difference: $${\color{green} 12 KB \space (0\%) }$$

src/models/sqlOutputContentProvider.ts

src/queryResult/queryResultWebViewController.ts

src/reactviews/pages/QueryResult/queryResultPane.tsx

src/reactviews/pages/QueryResult/queryResultStateProvider.tsx

src/sharedInterfaces/queryResult.ts

caohai · 2024-10-02T22:44:53Z

I noticed in the GIF that the column name under Results tab for query plan has a TODO tag and is not localized, I've included that fix in my PR here fa292ac

caohai · 2024-10-03T21:15:07Z

Can you add/update the screenshot/gif with a multi-batch query like this?

select * from sys.objects
go
select * from sys.objects
go

Here's what I'm seeing with your branch, looks like the query plan for the first query is being displayed, you can address multi-query support in later PRs.

One more thing I noticed: In ADS if you click the link for query plan xml in the result, a query plan editor tab will be opened, we can look into if we want to support this in vscode with the links. Currently clicking them doesn't do anything (as they are not wired up), feel free to ping me for the link handler if you need more info.

caohai · 2024-10-03T21:38:02Z

src/reactviews/pages/ExecutionPlan/executionPlanPage.tsx

    const classes = useStyles();
-    const provider = useContext(ExecutionPlanContext);
-    const loadState = provider?.state?.loadState ?? ApiStatus.Loading;
+    console.log(context);


Do we want to keep this log for prod or is it just for debugging?

aasimkhan30 · 2024-10-04T18:38:52Z

src/sharedInterfaces/queryResult.ts

@@ -11,11 +18,43 @@ export enum QueryResultLoadState {

 export interface QueryResultReactProvider {
    setResultTab: (tabId: QueryResultPaneTabs) => void;
+    /**


Maybe extract it out to a parent interface shared between the 2 webviews.
(execution plan and query result)

aasimkhan30 · 2024-10-04T18:40:26Z

src/sharedInterfaces/queryResult.ts

@@ -27,12 +66,37 @@ export interface QueryResultWebviewState {
    resultSetSummaries: { [key: number]: ResultSetSummary };
    messages: IMessage[];
    tabStates?: QueryResultTabStates;
+    isExecutionPlan?: boolean;
+    executionPlanState: {


Why not make an interface out of it and share it with the regular plan webview. Let's try avoid duplicate types since they tend to get out of sync with each other.

aasimkhan30 · 2024-10-04T18:43:02Z

src/sharedInterfaces/queryResult.ts

+        sqlPlanContent?: string;
+        executionPlan?: GetExecutionPlanResult;
+        executionPlanGraphs?: ExecutionPlanGraph[];
+        theme?: string;


Theming variable is provided by vscodeWebviewProvider. Please try to use that instead. Things like theming, telemetry and localization should be kept centralized as any bug fixes made to them will be applied everywhere.

aasimkhan30 · 2024-10-04T18:43:31Z

src/sharedInterfaces/queryResult.ts

 }

 export interface QueryResultReducers {
    setResultTab: {
        tabId: QueryResultPaneTabs;
    };
+    getExecutionPlan: {


Please centralize the reducers as well

aasimkhan30 · 2024-10-04T18:44:51Z

src/reactviews/pages/QueryResult/queryResultStateProvider.tsx

@@ -41,7 +45,50 @@ const QueryResultStateProvider: React.FC<QueryResultContextProps> = ({
                            tabId: tabId,
                        });
                    },
+                    getExecutionPlan: function (


If these are copied from the execution plan provider, it will be good to have one implementation of it.

aasimkhan30 · 2024-10-04T18:46:45Z

src/reactviews/pages/QueryResult/queryResultPane.tsx

@@ -85,6 +87,7 @@ const RESULTS = l10n.t("Results");
 const MESSAGES = l10n.t("Messages");
 const TIMESTAMP = l10n.t("Timestamp");
 const MESSAGE = l10n.t("Message");
+const QUERY_PLAN = l10n.t("Query Plan");


Maybe use the locFile in reactviews/common/locConstants.ts

@caohai and @laurennat, FYI, this approach won’t work because the webview initially loads with default strings. It then requests the extension to retrieve the current language file and updates the strings accordingly. Since these are constant variables, they cannot be updated dynamically after initialization.

Thanks @aasimkhan30 ! I created a PR fixing this #18136

aasimkhan30 · 2024-10-04T18:57:42Z

src/controllers/executionPlanWebviewController.ts

-                totalCost: 0,
+                executionPlanState: {
+                    sqlPlanContent: executionPlanContents,
+                    theme:


Please use the theme variable in vscodeWebviewContextProvider. This implementation already doesn't support high contrast theme.

aasimkhan30 · 2024-10-04T19:02:08Z

src/controllers/executionPlanWebviewController.ts

            };
        });
    }

    private async createExecutionPlanGraphs() {
-        if (!this.state.executionPlan) {
+        if (!this.state.executionPlanState.executionPlan) {


NIT: Why not make this function cleaner and just create a new variable and add all the properties and the end

var executionPlan = { //fill all the required stuff } this.state = { this.state, executionPlanState: executionPlan }

This will prevent you from repeating this.state.executionPlanState... every time.

aasimkhan30 · 2024-10-04T19:14:44Z

src/reactviews/pages/QueryResult/queryResultPane.tsx

+                            className={classes.queryResultContainer}
+                            style={{ height: "100%", minHeight: "300px" }}
+                        >
+                            <ExecutionPlanPage context={state} />


I don't think passing context in this way is the best design approach. It looks like you've special-cased the context variable in all the ExecutionPlan children components:

var context = context ?? useContext(ExecutionPlanProvider)

The purpose of using context is to avoid prop drilling (i.e., passing props down through multiple child components). A more effective way to handle this would be:

<ExecutionPlanContextProvider props={queryResultState.executionPlan} > <ExecutionPlanPage /> </ExecutionPlanContextProvider>

By properly initializing all the props in the provider, you wouldn't need to modify the existing ExecutionPlan components.

Benjin · 2024-10-04T21:15:45Z

src/controllers/executionPlanWebviewController.ts

@@ -104,7 +109,7 @@ export class ExecutionPlanWebviewController extends ReactWebviewPanelController<

            if (saveUri) {
                // Write the content to the new file
-                await vscode.workspace.fs.writeFile(
+                vscode.workspace.fs.writeFile(


if writeFile is async and we don't want to await it, then we should explicitly use the void keyword.

Benjin · 2024-10-04T21:16:12Z

src/controllers/executionPlanWebviewController.ts

@@ -118,49 +123,58 @@ export class ExecutionPlanWebviewController extends ReactWebviewPanelController<
                language: "xml",
            });

-            await vscode.window.showTextDocument(planXmlDoc);
+            vscode.window.showTextDocument(planXmlDoc);


void keyword

Benjin · 2024-10-04T21:16:22Z

src/controllers/executionPlanWebviewController.ts


            return state;
        });
        this.registerReducer("showQuery", async (state, payload) => {
-            await this.untitledSqlDocumentService.newQuery(payload.query);
+            this.untitledSqlDocumentService.newQuery(payload.query);


void keyword

laurennat added 10 commits September 24, 2024 10:48

exposed backend call for plan

a1299ec

execution plan from query

c1769c3

extract query text to use for execution plan

71e41ff

execution plan in query results pane: still needs some polishing and …

c49f6c8

…cleaning up

finished execution plan functionality in results pane; still need to …

a9acfee

…clean up code

merged main

0b0b740

little fixes and styling changes

106d109

removed uneccesary console statement

f47ec3c

merged main

fcb0ccc

localization

d3ab690

laurennat requested review from kburtram, aasimkhan30, cssuh, caohai, lewis-sanchez and Benjin as code owners October 1, 2024 22:33

fixed formatting

35f60fb