[FIX] Pivots: Recompute measure on indirect dependency update

Task: 5349782

Author: rrahir

src/functions/helper_lookup.ts

@@ -2,7 +2,7 @@ import { isZoneInside, positionToZone, zoneToXc } from "../helpers";
 import { _t } from "../translation";
 import { EvalContext, FunctionResultObject, Getters, Maybe, Range, UID } from "../types";
 import { CircularDependencyError, EvaluationError, InvalidReferenceError } from "../types/errors";
-import { PivotCoreDefinition, PivotCoreMeasure } from "../types/pivot";
+import { PivotCoreMeasure } from "../types/pivot";
 
 /**
  * Get the pivot ID from the formula pivot ID.
@@ -37,11 +37,12 @@ export function assertDomainLength(domain: Maybe<FunctionResultObject>[]) {
 
 export function addPivotDependencies(
   evalContext: EvalContext,
-  coreDefinition: PivotCoreDefinition,
+  pivotId: UID,
   forMeasures: PivotCoreMeasure[]
 ) {
   //TODO This function can be very costly when used with PIVOT.VALUE and PIVOT.HEADER
   const dependencies: Range[] = [];
+  const coreDefinition = evalContext.getters.getPivotCoreDefinition(pivotId);
 
   if (coreDefinition.type === "SPREADSHEET" && coreDefinition.dataSet) {
     const { sheetId, zone } = coreDefinition.dataSet;
@@ -62,8 +63,7 @@ export function addPivotDependencies(
 
   for (const measure of forMeasures) {
     if (measure.computedBy) {
-      const formula = evalContext.getters.getMeasureCompiledFormula(measure);
-      dependencies.push(...formula.dependencies.filter((range) => !range.invalidXc));
+      dependencies.push(...evalContext.getters.getMeasureFullDependencies(pivotId, measure));
     }
   }
   const originPosition = evalContext.__originCellPosition;

src/functions/module_lookup.ts

@@ -780,7 +780,7 @@ export const PIVOT_VALUE = {
 
     addPivotDependencies(
       this,
-      coreDefinition,
+      pivotId,
       coreDefinition.measures.filter((m) => m.id === _measure)
     );
     pivot.init({ reload: pivot.needsReevaluation });
@@ -824,8 +824,7 @@ export const PIVOT_HEADER = {
     const _pivotId = getPivotId(_pivotFormulaId, this.getters);
     assertDomainLength(domainArgs);
     const pivot = this.getters.getPivot(_pivotId);
-    const coreDefinition = this.getters.getPivotCoreDefinition(_pivotId);
-    addPivotDependencies(this, coreDefinition, []);
+    addPivotDependencies(this, _pivotId, []);
     pivot.init({ reload: pivot.needsReevaluation });
     const error = pivot.assertIsValid({ throwOnError: false });
     if (error) {
@@ -894,7 +893,7 @@ export const PIVOT = {
     const pivotId = getPivotId(_pivotFormulaId, this.getters);
     const pivot = this.getters.getPivot(pivotId);
     const coreDefinition = this.getters.getPivotCoreDefinition(pivotId);
-    addPivotDependencies(this, coreDefinition, coreDefinition.measures);
+    addPivotDependencies(this, pivotId, coreDefinition.measures);
     pivot.init({ reload: pivot.needsReevaluation });
     const error = pivot.assertIsValid({ throwOnError: false });
     if (error) {

src/helpers/pivot/pivot_presentation.ts

@@ -14,6 +14,7 @@ import {
   isMatrix,
 } from "../../types";
 import { CellErrorType, NotAvailableError } from "../../types/errors";
+import { UID } from "../../types/misc";
 import { deepEquals, removeDuplicates, transpose2dPOJO } from "../misc";
 import {
   NEXT_VALUE,
@@ -50,6 +51,7 @@ type DomainGroups<T> = { [colDomain: string]: { [rowDomain: string]: T } };
 export default function (PivotClass: PivotUIConstructor) {
   class PivotPresentationLayer extends PivotClass {
     private getters: Getters;
+    private pivotId: UID;
     private cache: Record<string, FunctionResultObject> = {};
     private rankAsc: CacheForMeasureAndField<DomainGroups<number> | undefined> = {};
     private rankDesc: CacheForMeasureAndField<DomainGroups<number> | undefined> = {};
@@ -60,9 +62,10 @@ export default function (PivotClass: PivotUIConstructor) {
       DomainGroups<number | undefined> | undefined
     > = {};
 
-    constructor(custom: ModelConfig["custom"], params: PivotParams) {
+    constructor(pivotId, custom: ModelConfig["custom"], params: PivotParams) {
       super(custom, params);
       this.getters = params.getters;
+      this.pivotId = pivotId;
     }
 
     markAsDirtyForEvaluation(): void {
@@ -117,7 +120,7 @@ export default function (PivotClass: PivotUIConstructor) {
           return handleError(error, measure.aggregator.toUpperCase());
         }
       }
-      const formula = this.getters.getMeasureCompiledFormula(measure);
+      const formula = this.getters.getMeasureCompiledFormula(this.pivotId, measure);
       const getSymbolValue = (symbolName: string) => {
         const { columns, rows } = this.definition;
         if (columns.find((col) => col.nameWithGranularity === symbolName)) {

src/plugins/core/pivot.ts

@@ -1,5 +1,5 @@
 import { compile } from "../../formulas";
-import { deepCopy, deepEquals } from "../../helpers";
+import { deepCopy, deepEquals, getCanonicalSymbolName } from "../../helpers";
 import { createPivotFormula, getMaxObjectId } from "../../helpers/pivot/pivot_helpers";
 import { pivotRegistry } from "../../helpers/pivot/pivot_registry";
 import { SpreadsheetPivotTable } from "../../helpers/pivot/table_spreadsheet_pivot";
@@ -22,11 +22,16 @@ interface Pivot {
   formulaId: string;
 }
 
+interface MeasureState {
+  formula: RangeCompiledFormula;
+  fullDependencies: Range[];
+}
+
 interface CoreState {
   nextFormulaId: number;
   pivots: Record<UID, Pivot | undefined>;
   formulaIds: Record<UID, string | undefined>;
-  compiledMeasureFormulas: Record<UID, Record<string, RangeCompiledFormula | undefined>>;
+  compiledMeasureFormulas: Record<UID, Record<string, MeasureState | undefined>>;
 }
 
 export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState {
@@ -39,14 +44,15 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
     "getMeasureCompiledFormula",
     "getPivotName",
     "isExistingPivot",
+    "getMeasureFullDependencies",
   ] as const;
 
   readonly nextFormulaId: number = 1;
   public readonly pivots: {
     [pivotId: UID]: Pivot | undefined;
   } = {};
   public readonly formulaIds: { [formulaId: UID]: UID | undefined } = {};
-  public readonly compiledMeasureFormulas: Record<UID, Record<string, RangeCompiledFormula>> = {};
+  public readonly compiledMeasureFormulas: Record<UID, Record<string, MeasureState>> = {};
 
   allowDispatch(cmd: CoreCommand) {
     switch (cmd.type) {
@@ -126,7 +132,7 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
       }
       case "UPDATE_PIVOT": {
         this.history.update("pivots", cmd.pivotId, "definition", deepCopy(cmd.pivot));
-        this.compileCalculatedMeasures(cmd.pivot.measures);
+        this.compileCalculatedMeasures(cmd.pivotId, cmd.pivot.measures);
         break;
       }
     }
@@ -145,9 +151,15 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
         this.history.update("pivots", pivotId, "definition", newDefinition);
       }
     }
-    for (const sheetId in this.compiledMeasureFormulas) {
-      for (const formulaString in this.compiledMeasureFormulas[sheetId]) {
-        const compiledFormula = this.compiledMeasureFormulas[sheetId][formulaString];
+    for (const pivotId in this.compiledMeasureFormulas) {
+      for (const measureId in this.compiledMeasureFormulas[pivotId]) {
+        // const compiledFormula = this.compiledMeasureFormulas[sheetId][formulaString];
+        const measure = this.pivots[pivotId]?.definition.measures.find((m) => m.id === measureId);
+        if (!measure || !measure.computedBy) {
+          continue;
+        }
+        const sheetId = measure.computedBy.sheetId;
+        const compiledFormula = this.compiledMeasureFormulas[pivotId][measureId].formula;
         const newDependencies: Range[] = [];
         for (const range of compiledFormula.dependencies) {
           const change = applyChange(range);
@@ -162,8 +174,9 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
           compiledFormula.tokens,
           newDependencies
         );
-        if (newFormulaString !== formulaString) {
-          this.replaceMeasureFormula(sheetId, formulaString, newFormulaString);
+        const oldFormulaString = measure.computedBy.formula;
+        if (newFormulaString !== oldFormulaString) {
+          this.replaceMeasureFormula(oldFormulaString, newFormulaString);
         }
       }
     }
@@ -210,12 +223,18 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
     return pivotId in this.pivots;
   }
 
-  getMeasureCompiledFormula(measure: PivotCoreMeasure): RangeCompiledFormula {
+  getMeasureCompiledFormula(pivotId: UID, measure: PivotCoreMeasure): RangeCompiledFormula {
     if (!measure.computedBy) {
       throw new Error(`Measure ${measure.fieldName} is not computed by formula`);
     }
-    const sheetId = measure.computedBy.sheetId;
-    return this.compiledMeasureFormulas[sheetId][measure.computedBy.formula];
+    return this.compiledMeasureFormulas[pivotId][measure.id].formula;
+  }
+
+  getMeasureFullDependencies(pivotId: UID, measure: PivotCoreMeasure): Range[] {
+    if (!measure.computedBy) {
+      throw new Error(`Measure ${measure.fieldName} is not computed by formula`);
+    }
+    return this.compiledMeasureFormulas[pivotId][measure.id].fullDependencies;
   }
 
   // -------------------------------------------------------------------------
@@ -228,27 +247,73 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
     formulaId = this.nextFormulaId.toString()
   ) {
     this.history.update("pivots", pivotId, { definition: deepCopy(pivot), formulaId });
-    this.compileCalculatedMeasures(pivot.measures);
+    this.compileCalculatedMeasures(pivotId, pivot.measures);
     this.history.update("formulaIds", formulaId, pivotId);
     this.history.update("nextFormulaId", this.nextFormulaId + 1);
   }
 
-  private compileCalculatedMeasures(measures: PivotCoreMeasure[]) {
+  private compileCalculatedMeasures(pivotId: UID, measures: PivotCoreMeasure[]) {
     for (const measure of measures) {
       if (measure.computedBy) {
-        const sheetId = measure.computedBy.sheetId;
         const compiledFormula = this.compileMeasureFormula(
           measure.computedBy.sheetId,
           measure.computedBy.formula
         );
         this.history.update(
           "compiledMeasureFormulas",
-          sheetId,
-          measure.computedBy.formula,
+          pivotId,
+          measure.id,
+          "formula",
           compiledFormula
         );
       }
     }
+    for (const measure of measures) {
+      if (measure.computedBy) {
+        const dependencies = this.computeMeasureFullDependencies(pivotId, measure);
+        this.history.update(
+          "compiledMeasureFormulas",
+          pivotId,
+          measure.id,
+          "fullDependencies",
+          dependencies
+        );
+      }
+    }
+  }
+
+  private computeMeasureFullDependencies(
+    pivotId: UID,
+    measure: PivotCoreMeasure,
+    exploredMeasures: Set<string> = new Set()
+  ): Range[] {
+    const rangeList: Range[] = [];
+    const definition = this.getPivotCoreDefinition(pivotId);
+    const formula = this.getMeasureCompiledFormula(pivotId, measure);
+    for (const token of formula.tokens) {
+      if (token.type !== "SYMBOL") {
+        continue;
+      }
+      const existingMeasure = definition.measures.find(
+        (measureCandidate) =>
+          getCanonicalSymbolName(measureCandidate.id) === token.value &&
+          measure.id !== measureCandidate.id
+      );
+
+      if (
+        !existingMeasure ||
+        exploredMeasures.has(existingMeasure.id) ||
+        !existingMeasure.computedBy
+      )
+        continue;
+
+      rangeList.push(
+        ...this.computeMeasureFullDependencies(pivotId, existingMeasure, exploredMeasures)
+      );
+    }
+    rangeList.push(...formula.dependencies.filter((range) => !range.invalidXc));
+    exploredMeasures.add(measure.id);
+    return rangeList;
   }
 
   private insertPivot(position: CellPosition, formulaId: UID, table: SpreadsheetPivotTable) {
@@ -314,22 +379,23 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
     };
   }
 
-  private replaceMeasureFormula(sheetId: UID, formulaString: string, newFormulaString: string) {
-    this.history.update("compiledMeasureFormulas", sheetId, formulaString, undefined);
-    this.history.update(
-      "compiledMeasureFormulas",
-      sheetId,
-      newFormulaString,
-      this.compileMeasureFormula(sheetId, newFormulaString)
-    );
+  private replaceMeasureFormula(formulaString: string, newFormulaString: string) {
+    // this.history.update("compiledMeasureFormulas", sheetId, formulaString, undefined);
+    // this.history.update(
+    //   "compiledMeasureFormulas",
+    //   sheetId,
+    //   newFormulaString,
+    //   this.compileMeasureFormula(sheetId, newFormulaString)
+    // );
     for (const pivotId in this.pivots) {
       const pivot = this.pivots[pivotId];
       if (!pivot) {
         continue;
       }
-
+      let shouldUpdateMeasures = false;
       for (const measure of pivot.definition.measures) {
         if (measure.computedBy?.formula === formulaString) {
+          shouldUpdateMeasures = true;
           const measureIndex = pivot.definition.measures.indexOf(measure);
           this.history.update(
             "pivots",
@@ -338,10 +404,13 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
             "measures",
             measureIndex,
             "computedBy",
-            { formula: newFormulaString, sheetId }
+            { formula: newFormulaString, sheetId: measure.computedBy.sheetId }
           );
         }
       }
+      if (shouldUpdateMeasures) {
+        this.compileCalculatedMeasures(pivotId, pivot.definition.measures);
+      }
     }
   }
 

src/plugins/ui_core_views/pivot_ui.ts

@@ -304,7 +304,7 @@ export class PivotUIPlugin extends UIPlugin {
     const definition = deepCopy(this.getters.getPivotCoreDefinition(pivotId));
     if (!(pivotId in this.pivots)) {
       const Pivot = withPivotPresentationLayer(pivotRegistry.get(definition.type).ui);
-      this.pivots[pivotId] = new Pivot(this.custom, { definition, getters: this.getters });
+      this.pivots[pivotId] = new Pivot(pivotId, this.custom, { definition, getters: this.getters });
     } else if (recreate) {
       this.pivots[pivotId].onDefinitionChange(definition);
     }

tests/pivots/pivot_calculated_measure.test.ts

@@ -1188,3 +1188,34 @@ describe("Pivot calculated measure", () => {
     expect(getEvaluatedCell(model, "A6").value).toEqual(0);
   });
 });
+
+test("measure takes indirect dependency into account for recalculation", () => {
+  const grid = {
+    A1: "Customer",
+    A2: "Alice",
+    A3: "42",
+    A4: '=PIVOT.VALUE(1, "calculated")',
+    A5: '=PIVOT.VALUE(1, "basedOnCalculated")',
+  };
+  const model = createModelFromGrid(grid);
+  const sheetId = model.getters.getActiveSheetId();
+  addPivot(model, "A1:A2", {
+    measures: [
+      {
+        id: "calculated",
+        fieldName: "calculated",
+        aggregator: "sum",
+        computedBy: { formula: "=A3", sheetId },
+      },
+      {
+        id: "basedOnCalculated",
+        fieldName: "basedOnCalculated",
+        aggregator: "sum",
+        computedBy: { formula: "=calculated", sheetId },
+      },
+    ],
+  });
+  setCellContent(model, "A3", "43");
+  expect(getEvaluatedCell(model, "A4").value).toEqual(43);
+  expect(getEvaluatedCell(model, "A5").value).toEqual(43);
+});