Actually report bad join orders in recursions

nickrolfe · nickrolfe · commit a1552894fb4f · 2025-04-25T12:53:35.000+01:00
diff --git a/extensions/ql-vscode/src/log-insights/join-order.ts b/extensions/ql-vscode/src/log-insights/join-order.ts
@@ -19,23 +19,6 @@ function safeMax(it?: Iterable<number>) {
   return Number.isFinite(m) ? m : 0;
 }
 
-/**
- * Compute a key for the maps that that is sent to report generation.
- * Should only be used on events that are known to define queryCausingWork.
- */
-function makeKey(
-  queryCausingWork: string | undefined,
-  predicate: string,
-  suffix = "",
-): string {
-  if (queryCausingWork === undefined) {
-    throw new Error(
-      "queryCausingWork was not defined on an event we expected it to be defined for!",
-    );
-  }
-  return `${queryCausingWork}:${predicate}${suffix ? ` ${suffix}` : ""}`;
-}
-
 function getDependentPredicates(operations: string[]): string[] {
   const id = String.raw`[0-9a-zA-Z:#_\./]+`;
   const idWithAngleBrackets = String.raw`[0-9a-zA-Z:#_<>\./]+`;
@@ -128,14 +111,6 @@ function pointwiseSum(
   return result;
 }
 
-function pushValue<K, V>(m: Map<K, V[]>, k: K, v: V) {
-  if (!m.has(k)) {
-    m.set(k, []);
-  }
-  m.get(k)!.push(v);
-  return m;
-}
-
 function computeJoinOrderBadness(
   maxTupleCount: number,
   maxDependentPredicateSize: number,
@@ -161,11 +136,6 @@ class JoinOrderScanner implements EvaluationLogScanner {
     string,
     Array<ComputeRecursive | InLayer>
   >();
-  // Map a key of the form 'query-with-demand : predicate name' to its badness input.
-  private readonly maxTupleCountMap = new Map<string, number[]>();
-  private readonly resultSizeMap = new Map<string, number[]>();
-  private readonly maxDependentPredicateSizeMap = new Map<string, number[]>();
-  private readonly joinOrderMetricMap = new Map<string, number>();
 
   constructor(
     private readonly problemReporter: EvaluationLogProblemReporter,
@@ -216,27 +186,6 @@ class JoinOrderScanner implements EvaluationLogScanner {
     }
   }
 
-  private reportProblemIfNecessary(
-    event: SummaryEvent,
-    iteration: number,
-    metric: number,
-  ): void {
-    if (metric >= this.warningThreshold) {
-      this.problemReporter.reportProblem(
-        event.predicateName,
-        event.raHash,
-        iteration,
-        `Relation '${
-          event.predicateName
-        }' has an inefficient join order. Its join order metric is ${metric.toFixed(
-          2,
-        )}, which is larger than the threshold of ${this.warningThreshold.toFixed(
-          2,
-        )}.`,
-      );
-    }
-  }
-
   private computeBadnessMetric(event: SummaryEvent): void {
     if (
       event.completionType !== undefined &&
@@ -252,28 +201,32 @@ class JoinOrderScanner implements EvaluationLogScanner {
         }
         // Compute the badness metric for a non-recursive predicate. The metric in this case is defined as:
         // badness = (max tuple count in the pipeline) / (largest predicate this pipeline depends on)
-        const key = makeKey(event.queryCausingWork, event.predicateName);
         const resultSize = event.resultSize;
 
         // There is only one entry in `pipelineRuns` if it's a non-recursive predicate.
         const { maxTupleCount, maxDependentPredicateSize } =
           this.badnessInputsForNonRecursiveDelta(event.pipelineRuns[0], event);
 
         if (maxDependentPredicateSize > 0) {
-          pushValue(this.maxTupleCountMap, key, maxTupleCount);
-          pushValue(this.resultSizeMap, key, resultSize);
-          pushValue(
-            this.maxDependentPredicateSizeMap,
-            key,
-            maxDependentPredicateSize,
-          );
           const metric = computeJoinOrderBadness(
             maxTupleCount,
             maxDependentPredicateSize,
             resultSize!,
           );
-          this.joinOrderMetricMap.set(key, metric);
-          this.reportProblemIfNecessary(event, 0, metric);
+          if (metric >= this.warningThreshold) {
+            const message = `'${
+              event.predicateName
+            }' has an inefficient join order. Its join order metric is ${metric.toFixed(
+              2,
+            )}, which is larger than the threshold of ${this.warningThreshold.toFixed(
+              2,
+            )}.`;
+            this.problemReporter.reportProblemNonRecursive(
+              event.predicateName,
+              event.raHash,
+              message,
+            );
+          }
         }
         break;
       }
@@ -282,39 +235,39 @@ class JoinOrderScanner implements EvaluationLogScanner {
         // Compute the badness metric for a recursive predicate for each ordering.
         const sccMetricInput = this.badnessInputsForRecursiveDelta(event);
         // Loop through each predicate in the SCC
-        sccMetricInput.forEach((buckets, predicate) => {
-          // Loop through each ordering of the predicate
-          buckets.forEach((bucket, raReference) => {
-            // Format the key as demanding-query:name (ordering)
-            const key = makeKey(
-              event.queryCausingWork,
-              predicate,
-              `(${raReference})`,
-            );
-            const maxTupleCount = Math.max(...bucket.tupleCounts);
-            const resultSize = bucket.resultSize;
-            const maxDependentPredicateSize = Math.max(
-              ...bucket.dependentPredicateSizes.values(),
-            );
-
-            if (maxDependentPredicateSize > 0) {
-              pushValue(this.maxTupleCountMap, key, maxTupleCount);
-              pushValue(this.resultSizeMap, key, resultSize);
-              pushValue(
-                this.maxDependentPredicateSizeMap,
-                key,
-                maxDependentPredicateSize,
+        sccMetricInput.forEach((hashToOrderToBucket, predicateName) => {
+          hashToOrderToBucket.forEach((orderToBucket, raHash) => {
+            // Loop through each ordering of the predicate.
+            orderToBucket.forEach((bucket, raReference) => {
+              const maxDependentPredicateSize = Math.max(
+                ...bucket.dependentPredicateSizes.values(),
               );
-              const metric = computeJoinOrderBadness(
-                maxTupleCount,
-                maxDependentPredicateSize,
-                resultSize,
-              );
-              const oldMetric = this.joinOrderMetricMap.get(key);
-              if (oldMetric === undefined || metric > oldMetric) {
-                this.joinOrderMetricMap.set(key, metric);
+
+              if (maxDependentPredicateSize > 0) {
+                const maxTupleCount = Math.max(...bucket.tupleCounts);
+                const resultSize = bucket.resultSize;
+                const metric = computeJoinOrderBadness(
+                  maxTupleCount,
+                  maxDependentPredicateSize,
+                  resultSize,
+                );
+                if (metric >= this.warningThreshold) {
+                  const message = `The ${raReference} pipeline for '${
+                    predicateName
+                  }' has an inefficient join order. Its join order metric is ${metric.toFixed(
+                    2,
+                  )}, which is larger than the threshold of ${this.warningThreshold.toFixed(
+                    2,
+                  )}.`;
+                  this.problemReporter.reportProblemForRecursionSummary(
+                    predicateName,
+                    raHash,
+                    raReference,
+                    message,
+                  );
+                }
               }
-            }
+            });
           });
         });
         break;
@@ -457,20 +410,28 @@ class JoinOrderScanner implements EvaluationLogScanner {
    */
   private badnessInputsForRecursiveDelta(
     event: ComputeRecursive,
-  ): Map<string, Map<string, Bucket>> {
-    // nameToOrderToBucket : predicate name -> ordering (i.e., standard, order_500000, etc.) -> bucket
-    const nameToOrderToBucket = new Map<string, Map<string, Bucket>>();
+  ): Map<string, Map<string, Map<string, Bucket>>> {
+    // nameToHashToOrderToBucket : predicate name -> RA hash -> ordering (i.e., standard, order_500000, etc.) -> bucket
+    const nameToHashToOrderToBucket = new Map<
+      string,
+      Map<string, Map<string, Bucket>>
+    >();
 
     // Iterate through the SCC and compute the metric inputs
     this.iterateSCC(event, (inLayerEvent, run, iteration) => {
       const raReference = run.raReference;
       const predicateName = inLayerEvent.predicateName;
-      if (!nameToOrderToBucket.has(predicateName)) {
-        nameToOrderToBucket.set(predicateName, new Map());
+      if (!nameToHashToOrderToBucket.has(predicateName)) {
+        nameToHashToOrderToBucket.set(predicateName, new Map());
+      }
+      const hashToOrderToBucket = nameToHashToOrderToBucket.get(predicateName)!;
+      const raHash = inLayerEvent.raHash;
+      if (!hashToOrderToBucket.has(raHash)) {
+        hashToOrderToBucket.set(raHash, new Map());
       }
-      const orderTobucket = nameToOrderToBucket.get(predicateName)!;
-      if (!orderTobucket.has(raReference)) {
-        orderTobucket.set(raReference, {
+      const orderToBucket = hashToOrderToBucket.get(raHash)!;
+      if (!orderToBucket.has(raReference)) {
+        orderToBucket.set(raReference, {
           tupleCounts: new Int32Array(0),
           resultSize: 0,
           dependentPredicateSizes: new Map(),
@@ -484,7 +445,7 @@ class JoinOrderScanner implements EvaluationLogScanner {
         iteration,
       );
 
-      const bucket = orderTobucket.get(raReference)!;
+      const bucket = orderToBucket.get(raReference)!;
       // Pointwise sum the tuple counts
       const newTupleCounts = pointwiseSum(
         bucket.tupleCounts,
@@ -504,13 +465,13 @@ class JoinOrderScanner implements EvaluationLogScanner {
         );
       }
 
-      orderTobucket.set(raReference, {
+      orderToBucket.set(raReference, {
         tupleCounts: newTupleCounts,
         resultSize,
         dependentPredicateSizes: newDependentPredicateSizes,
       });
     });
-    return nameToOrderToBucket;
+    return nameToHashToOrderToBucket;
   }
 }
 
diff --git a/extensions/ql-vscode/src/log-insights/log-scanner-service.ts b/extensions/ql-vscode/src/log-insights/log-scanner-service.ts
@@ -28,17 +28,41 @@ class ProblemReporter implements EvaluationLogProblemReporter {
 
   constructor(private readonly symbols: SummarySymbols | undefined) {}
 
-  public reportProblem(
+  public reportProblemNonRecursive(
     predicateName: string,
     raHash: string,
-    iteration: number,
     message: string,
   ): void {
     const nameWithHash = predicateSymbolKey(predicateName, raHash);
     const predicateSymbol = this.symbols?.predicates[nameWithHash];
     let predicateInfo: PipelineInfo | undefined = undefined;
     if (predicateSymbol !== undefined) {
-      predicateInfo = predicateSymbol.iterations[iteration];
+      predicateInfo = predicateSymbol.iterations[0];
+    }
+    if (predicateInfo !== undefined) {
+      const range = new Range(
+        predicateInfo.raStartLine,
+        0,
+        predicateInfo.raEndLine + 1,
+        0,
+      );
+      this.diagnostics.push(
+        new Diagnostic(range, message, DiagnosticSeverity.Error),
+      );
+    }
+  }
+
+  public reportProblemForRecursionSummary(
+    predicateName: string,
+    raHash: string,
+    order: string,
+    message: string,
+  ): void {
+    const nameWithHash = predicateSymbolKey(predicateName, raHash);
+    const predicateSymbol = this.symbols?.predicates[nameWithHash];
+    let predicateInfo: PipelineInfo | undefined = undefined;
+    if (predicateSymbol !== undefined) {
+      predicateInfo = predicateSymbol.recursionSummaries[order];
     }
     if (predicateInfo !== undefined) {
       const range = new Range(
diff --git a/extensions/ql-vscode/src/log-insights/log-scanner.ts b/extensions/ql-vscode/src/log-insights/log-scanner.ts
@@ -8,18 +8,30 @@ import type { SummaryEvent } from "./log-summary";
  */
 export interface EvaluationLogProblemReporter {
   /**
-   * Report a potential problem detected in the evaluation log.
+   * Report a potential problem detected in the evaluation log for a non-recursive predicate.
    *
    * @param predicateName The mangled name of the predicate with the problem.
    * @param raHash The RA hash of the predicate with the problem.
-   * @param iteration The iteration number with the problem. For a non-recursive predicate, this
-   * must be zero.
    * @param message The problem message.
    */
-  reportProblem(
+  reportProblemNonRecursive(
     predicateName: string,
     raHash: string,
-    iteration: number,
+    message: string,
+  ): void;
+
+  /**
+   * Report a potential problem detected in the evaluation log for the summary of a recursive pipeline.
+   *
+   * @param predicateName The mangled name of the predicate with the problem.
+   * @param raHash The RA hash of the predicate with the problem.
+   * @param order The particular order (pipeline name) that had the problem.
+   * @param message The problem message.
+   */
+  reportProblemForRecursionSummary(
+    predicateName: string,
+    raHash: string,
+    order: string,
     message: string,
   ): void;
 
diff --git a/extensions/ql-vscode/src/log-insights/summary-parser.ts b/extensions/ql-vscode/src/log-insights/summary-parser.ts
@@ -18,6 +18,11 @@ interface PredicateSymbol {
    * `PipelineInfo` for each iteration. A non-recursive predicate will have a single iteration `0`.
    */
   iterations: Record<number, PipelineInfo>;
+
+  /**
+   * `PipelineInfo` for each order, summarised for all iterations that used that order. Empty for non-recursive predicates.
+   */
+  recursionSummaries: Record<string, PipelineInfo>;
 }
 
 /**
@@ -105,6 +110,7 @@ async function generateSummarySymbols(
           if (symbol === undefined) {
             symbol = {
               iterations: {},
+              recursionSummaries: {},
             };
             symbols.predicates[predicateName] = symbol;
           }
