Skip to content

Commit

Permalink
[FIX] evaluation: Ensure dependency invalidation for array formulas
Browse files Browse the repository at this point in the history
How to reproduce:
-> see the attached test

What is happening?
During the evaluation of SUMIFS, it will fetch its ranges.
First it will first fetch an empty range (E4:E7)  since MUNIT hasn't
been evaluated yet.
Afterwards, it will fetch H4, which in turn will request C4
and therefore force the evaluation of the spreaded function MUNIT.

AT this point, there is a disparity of values because the values
of the range (E4:E7) have changed.
To address those situations, the evaluator invalidates the dependencies
of all the spreaded zone. Which means marking A1 to be recomputed.

Unfortunately, A1 is still being evaluated and since #4589 (specifically
commit 8d22e86) we remove the cell from the queue after we evaluted it.

This revision changes the approach taken in 8d22e86 to allow
legitimate invalidations take place while avoiding useless
reevaluations.

Task: 4252800
  • Loading branch information
rrahir committed Nov 15, 2024
1 parent 19b7478 commit b2aa847
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 6 deletions.
22 changes: 20 additions & 2 deletions src/functions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,17 @@ import {
Arg,
CellValue,
ComputeFunction,
DEFAULT_LOCALE,
EvalContext,
FPayload,
FunctionDescription,
Matrix,
isMatrix,
} from "../types";
import { BadExpressionError, EvaluationError } from "../types/errors";
import { addMetaInfoFromArg, validateArguments } from "./arguments";
import { isEvaluationError, matrixForEach, matrixMap } from "./helpers";
import { addMetaInfoFromArg, arg, validateArguments } from "./arguments";
import { toScalar } from "./helper_matrices";
import { isEvaluationError, matrixForEach, matrixMap, toNumber } from "./helpers";
import * as array from "./module_array";
import * as misc from "./module_custom";
import * as database from "./module_database";
Expand Down Expand Up @@ -222,3 +224,19 @@ for (let category of categories) {
functionRegistry.add(name, { isExported: false, ...addDescr });
}
}

functionRegistry.add("MFILL", {
description: "Return an n*n matrix filled with n.",
args: [
arg("n (number)", "number of column of the matrix"),
arg("m (number)", "number of row of the matrix"),
arg("v (number)", "value to fill matrix"),
],
returns: ["RANGE<NUMBER>"],
compute: function (n, m, v): number[][] {
const _n = toNumber(toScalar(n), DEFAULT_LOCALE);
const _m = toNumber(toScalar(m), DEFAULT_LOCALE);
const _v = toNumber(toScalar(v), DEFAULT_LOCALE);
return Array.from({ length: _n }, (_, i) => Array.from({ length: _m }, (_, j) => _v));
},
});
5 changes: 4 additions & 1 deletion src/plugins/ui_core_views/cell_evaluation/evaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,9 @@ export class Evaluator {
}
for (let i = 0; i < positions.length; ++i) {
const position = positions[i];
if (this.nextPositionsToUpdate.has(position)) {
continue;
}
const evaluatedCell = this.computeCell(position);
if (evaluatedCell !== EMPTY_CELL) {
this.evaluatedCells.set(position, evaluatedCell);
Expand Down Expand Up @@ -267,7 +270,6 @@ export class Evaluator {
return createEvaluatedCell(e);
} finally {
this.cellsBeingComputed.delete(cellId);
this.nextPositionsToUpdate.delete(position);
}
}

Expand Down Expand Up @@ -346,6 +348,7 @@ export class Evaluator {
{ sheetId, zone: rightPartZone },
{ sheetId, zone: leftColumnZone },
]);
invalidatedPositions.delete(arrayFormulaPosition);
this.nextPositionsToUpdate.addMany(invalidatedPositions);
}

Expand Down
19 changes: 16 additions & 3 deletions tests/evaluation/evaluation_formula_array.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,19 @@ describe("evaluate formulas that return an array", () => {
expect(mockCompute).toHaveBeenCalledTimes(3);
});

test("Formula depending on array formula is reevaluated when the array formula result changes", () => {
const model = new Model();
setCellContent(model, "A1", "=sumifs(E4:E7,H4:H7,1)");
setCellContent(model, "C4", "=MUNIT(4)");
setCellContent(model, "H4", "=C4");
setCellContent(model, "H6", "=E6");
expect(getEvaluatedCell(model, "A1").value).toBe(1);

// Force a reevaluation to avoid the incremental evaluation following each update_cell
model.dispatch("EVALUATE_CELLS");
expect(getEvaluatedCell(model, "A1").value).toBe(1);
});

test("Spreaded formulas with range deps invalidate only once the dependencies of themselves", () => {
let c = 0;
functionRegistry.add("INCREMENTONEVAL", {
Expand Down Expand Up @@ -696,9 +709,9 @@ describe("evaluate formulas that return an array", () => {

setCellContent(model, "A1", "2");

expect(getEvaluatedCell(model, "B1").value).toBe("#ERROR");
expect(getEvaluatedCell(model, "B2").value).toBe(null);
expect(getEvaluatedCell(model, "B3").value).toBe(0);
expect(getEvaluatedCell(model, "B1").value).toBe(42);
expect(getEvaluatedCell(model, "B2").value).toBe(42);
expect(getEvaluatedCell(model, "B3").value).toBe(42);

expect(getEvaluatedCell(model, "A3").value).toBe("#ERROR");
});
Expand Down

0 comments on commit b2aa847

Please sign in to comment.