Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIX] pivot: finer grained cycle detection #5026

Closed
wants to merge 1 commit into from
Closed

[FIX] pivot: finer grained cycle detection #5026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/functions/helper_lookup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { zoneToXc } from "../helpers";
import { _t } from "../translation";
import { EvalContext, FunctionResultObject, Getters, Maybe, Range, UID } from "../types";
import { EvaluationError, InvalidReferenceError } from "../types/errors";
import { PivotCoreDefinition } from "../types/pivot";
import { PivotCoreDefinition, PivotCoreMeasure } from "../types/pivot";

/**
* Get the pivot ID from the formula pivot ID.
Expand Down Expand Up @@ -37,7 +37,8 @@ export function assertDomainLength(domain: Maybe<FunctionResultObject>[]) {

export function addPivotDependencies(
evalContext: EvalContext,
coreDefinition: PivotCoreDefinition
coreDefinition: PivotCoreDefinition,
forMeasures: PivotCoreMeasure[]
) {
//TODO This function can be very costly when used with PIVOT.VALUE and PIVOT.HEADER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated: what's the state of this TODO ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it's still a todo until we find it to be a real bottleneck ;-)

const dependencies: Range[] = [];
Expand All @@ -52,7 +53,7 @@ export function addPivotDependencies(
dependencies.push(range);
}

for (const measure of coreDefinition.measures) {
for (const measure of forMeasures) {
if (measure.computedBy) {
const formula = evalContext.getters.getMeasureCompiledFormula(measure);
dependencies.push(...formula.dependencies.filter((range) => !range.invalidXc));
Expand Down
10 changes: 7 additions & 3 deletions src/functions/module_lookup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,11 @@ export const PIVOT_VALUE = {
const pivot = this.getters.getPivot(pivotId);
const coreDefinition = this.getters.getPivotCoreDefinition(pivotId);

addPivotDependencies(this, coreDefinition);
addPivotDependencies(
this,
coreDefinition,
coreDefinition.measures.filter((m) => m.id === _measure)
);
pivot.init({ reload: pivot.needsReevaluation });
const error = pivot.assertIsValid({ throwOnError: false });
if (error) {
Expand Down Expand Up @@ -747,7 +751,7 @@ export const PIVOT_HEADER = {
assertDomainLength(domainArgs);
const pivot = this.getters.getPivot(_pivotId);
const coreDefinition = this.getters.getPivotCoreDefinition(_pivotId);
addPivotDependencies(this, coreDefinition);
addPivotDependencies(this, coreDefinition, []);
pivot.init({ reload: pivot.needsReevaluation });
const error = pivot.assertIsValid({ throwOnError: false });
if (error) {
Expand Down Expand Up @@ -813,7 +817,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);
addPivotDependencies(this, coreDefinition, coreDefinition.measures);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now we can reference headers of PIVOT.HEADER() but not headers originating from a spread pivot formula if I understand the code correctly. Do you think there's any way we can make that work ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there's any way we can make that work ?

yes, but it'd be quite difficult I think. Every cell would need to be computed in the correct dependency order, when they are needed.

pivot.init({ reload: pivot.needsReevaluation });
const error = pivot.assertIsValid({ throwOnError: false });
if (error) {
Expand Down
48 changes: 48 additions & 0 deletions tests/pivots/pivot_calculated_measure.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,54 @@ describe("Pivot calculated measure", () => {
expect(getEvaluatedCell(model, "A3").message).toEqual("Circular reference");
});

test("can depend on a cell containing another header", () => {
const grid = {
A1: '=PIVOT.HEADER(1, "Customer", "Alice")', // not referenced by the computed measure
A10: '=PIVOT.HEADER(1, "Customer", "Alice")', // referenced by the computed measure
A2: "Customer",
A3: "Alice",
};
const model = createModelFromGrid(grid);
const sheetId = model.getters.getActiveSheetId();
addPivot(model, "A2:A3", {
rows: [{ fieldName: "Customer" }],
measures: [
{
id: "calculated",
fieldName: "calculated",
aggregator: "sum",
computedBy: { formula: "=A10", sheetId },
},
],
});
expect(getEvaluatedCell(model, "A10").value).toEqual("Alice");
expect(getEvaluatedCell(model, "A1").value).toEqual("Alice");
});

test("can depend on a cell containing another value", () => {
const grid = {
A1: '=PIVOT.VALUE(1, "Customer")', // not referenced by the computed measure
A10: '=PIVOT.VALUE(1, "Customer")', // referenced by the computed measure
A2: "Customer",
A3: "Alice",
};
const model = createModelFromGrid(grid);
const sheetId = model.getters.getActiveSheetId();
addPivot(model, "A2:A3", {
measures: [
{ id: "Customer", fieldName: "Customer", aggregator: "sum" },
{
id: "calculated",
fieldName: "calculated",
aggregator: "sum",
computedBy: { formula: "=A10", sheetId },
},
],
});
expect(getEvaluatedCell(model, "A10").value).toEqual(0);
expect(getEvaluatedCell(model, "A1").value).toEqual(0);
});

test("measures symbols are scoped to the formula", () => {
// prettier-ignore
const grid = {
Expand Down