Skip to content

Commit

Permalink
[FIX] evaluation: handle throwing error on each vectorized position
Browse files Browse the repository at this point in the history
Before this commit, if an error was throw when evaluating a
vectorized formula, then the evaluation stopped and only the
cell with the formula was marked as an error.
No other values ​​were spread.

This behavior is functionally false, because depending on the
arguments passed, it is possible that on this vector position,
the evaluation does not return an error.

This commit changes that by making sure to try for each vector
position if the evaluation works. Even though it only spreads
errors in certain cases.

closes #4798

Task: 4091276
X-original-commit: 29775e1
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
  • Loading branch information
laa-odoo committed Aug 8, 2024
1 parent 38f2906 commit d2c5975
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 15 deletions.
31 changes: 16 additions & 15 deletions src/functions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,6 @@ function createComputeFunction(
descr: FunctionDescription,
functionName: string
): ComputeFunction<Matrix<FunctionResultObject> | FunctionResultObject> {
function runtimeCompute(
this: EvalContext,
...args: Arg[]
): Matrix<FunctionResultObject> | FunctionResultObject {
try {
return vectorizedCompute.apply(this, args);
} catch (e) {
return handleError(e, functionName);
}
}

function vectorizedCompute(
this: EvalContext,
...args: Arg[]
Expand Down Expand Up @@ -174,7 +163,8 @@ function createComputeFunction(
if (!isMatrix(arg) && argDefinition.acceptMatrixOnly) {
throw new BadExpressionError(
_t(
"Function [[FUNCTION_NAME]] expects the parameter '%s' to be reference to a cell or range.",
"Function %s expects the parameter '%s' to be reference to a cell or range.",
functionName,
(i + 1).toString()
)
);
Expand All @@ -184,7 +174,7 @@ function createComputeFunction(

if (countVectorizableCol === 1 && countVectorizableRow === 1) {
// either this function is not vectorized or it ends up with a 1x1 dimension
return computeFunctionToObject.apply(this, args);
return errorHandlingCompute.apply(this, args);
}

const getArgOffset: (i: number, j: number) => Arg[] = (i, j) =>
Expand All @@ -205,7 +195,7 @@ function createComputeFunction(
if (col > vectorizableColLimit - 1 || row > vectorizableRowLimit - 1) {
return notAvailableError;
}
const singleCellComputeResult = computeFunctionToObject.apply(this, getArgOffset(col, row));
const singleCellComputeResult = errorHandlingCompute.apply(this, getArgOffset(col, row));
// In the case where the user tries to vectorize arguments of an array formula, we will get an
// array for every combination of the vectorized arguments, which will lead to a 3D matrix and
// we won't be able to return the values.
Expand All @@ -221,6 +211,17 @@ function createComputeFunction(
});
}

function errorHandlingCompute(
this: EvalContext,
...args: Arg[]
): Matrix<FunctionResultObject> | FunctionResultObject {
try {
return computeFunctionToObject.apply(this, args);
} catch (e) {
return handleError(e, functionName);
}
}

function computeFunctionToObject(
this: EvalContext,
...args: Arg[]
Expand All @@ -245,7 +246,7 @@ function createComputeFunction(
return matrixMap(result as Matrix<CellValue>, (row) => ({ value: row }));
}

return runtimeCompute;
return vectorizedCompute;
}

export function handleError(e: unknown, functionName: string): FunctionResultObject {
Expand Down
19 changes: 19 additions & 0 deletions tests/functions/vectorization.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,25 @@ describe("vectorization", () => {
expect(checkFunctionDoesntSpreadBeyondRange(model, "D1:E2")).toBeTruthy();
});

test("function which throws an error during the evaluation of the position of one of its vectors, will apply the error to this position and continue the evaluation for each vector position", () => {
// prettier-ignore
const grid = {
A1: "A1", B1: "B1",
A2: "A2", B2: "#ERROR",
A3: "A3", B3: "B3",
};
const model = createModelFromGrid(grid);
setCellContent(model, "D1", '=FUNCTION.WITHOUT.RANGE.ARGS("this is ", B1:B3)');
expect(getRangeValuesAsMatrix(model, "D1:D3")).toEqual([
["this is B1"],
["#ERROR"],
["this is B3"],
]);

setCellContent(model, "D1", '=FUNCTION.WITHOUT.RANGE.ARGS("#ERROR", A1:A3)');
expect(getRangeValuesAsMatrix(model, "D1:D3")).toEqual([["#ERROR"], ["#ERROR"], ["#ERROR"]]);
});

test("binary operators should always accept vectors", () => {
// mean binary operators args should always be simple args
for (let op in OPERATOR_MAP) {
Expand Down

0 comments on commit d2c5975

Please sign in to comment.