Skip to content

[FIX] Pivots: Recompute measure on indirect dependency update #7610

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
Open
rrahir wants to merge 3 commits into
base: 18.0
Choose a base branch
from
Open

[FIX] Pivots: Recompute measure on indirect dependency update #7610

rrahir wants to merge 3 commits into from

Conversation

@rrahir
Copy link
Collaborator

@rrahir rrahir commented Dec 5, 2025

Description:

description of this task, what is implemented and why it is implemented that way.

Task: 5349782

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Dec 5, 2025

Pull request status dashboard

@rrahir rrahir force-pushed the 18.0-pivot-measure-indirect-dependencies-rar branch from 1851586 to 8bb1d5b Compare December 5, 2025 14:33
hokolomopo
hokolomopo reviewed Dec 17, 2025
View reviewed changes
Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

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

Need a rebase to remove the first two commits 🙂

Maybe a dumb idea, but instead of bothering computing all the exact dependencies for each measure, couldn't we just have the getter getMeasureFullDependencies always return all of the dependencies of all of the measures of the pivot ? It would simplify the code a lot, and I'm not sure it would really impact the performances in practice.

src/plugins/core/pivot.ts
Comment on lines +309 to +314
rangeList.push(
...this.computeMeasureFullDependencies(pivotId, existingMeasure, exploredMeasures)
);
Copy link
Contributor

@hokolomopo hokolomopo Dec 17, 2025

Choose a reason for hiding this comment

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

this will create an infinite loop if we have cyclic dependencies in measures no ?

src/plugins/core/pivot.ts Outdated
Comment on lines 306 to 309
)
continue;

Copy link
Contributor

@hokolomopo hokolomopo Dec 17, 2025

Choose a reason for hiding this comment

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

nitpick: brackets

src/plugins/core/pivot.ts Outdated
Comment on lines 382 to 388
// this.history.update("compiledMeasureFormulas", sheetId, formulaString, undefined);
// this.history.update(
// "compiledMeasureFormulas",
// sheetId,
// newFormulaString,
// this.compileMeasureFormula(sheetId, newFormulaString)
// );
Copy link
Contributor

@hokolomopo hokolomopo Dec 17, 2025

Choose a reason for hiding this comment

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

comment

src/plugins/core/pivot.ts

interface MeasureState {
formula: RangeCompiledFormula;
fullDependencies: Range[];
Copy link
Contributor

@hokolomopo hokolomopo Dec 17, 2025

Choose a reason for hiding this comment

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

I'd remove the "full" in the name of this/the getter

src/plugins/core/pivot.ts
Comment on lines +270 to +273
for (const measure of measures) {
if (measure.computedBy) {
const dependencies = this.computeMeasureFullDependencies(pivotId, measure);
this.history.update(
Copy link
Contributor

@hokolomopo hokolomopo Dec 17, 2025

Choose a reason for hiding this comment

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

Can probably be done in the same loop as the complied formula, if computeMeasureFullDependencies returned the whole MeasureState object.

Copy link
Collaborator Author

@rrahir rrahir Dec 22, 2025

Choose a reason for hiding this comment

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

if measure1 refers to measure2 and the formula of measure2 was not already compiled, you'll have a problem ^^

regarding the generic remark "always return all of the dependencies of all of the measures of the pivot " this would only have an impact on split pivot formulas which would end up having the dependencies of every measure, even those that have 0 relation to it. I don't know if it's a deal breaker

@rrahir rrahir force-pushed the 18.0-pivot-measure-indirect-dependencies-rar branch 3 times, most recently from b68a741 to e6ca467 Compare December 22, 2025 16:19
@rrahir
Copy link
Collaborator Author

rrahir commented Dec 22, 2025

@hokolomopo I have implemented your suggestion, we now store a single array of range dependecies for every pivot. it will trigger the unwanted recomputation of static pivot formulas but should not be too troublesome

LucasLefevre
LucasLefevre reviewed Dec 30, 2025
View reviewed changes
src/plugins/core/pivot.ts Outdated
formulaIds: Record<UID, string | undefined>;
compiledMeasureFormulas: Record<UID, Record<string, RangeCompiledFormula | undefined>>;
compiledMeasureFormulas: Record<UID, Record<string, RangeCompiledFormula>>;
pivotMeasureDependencies: Record<UID, Range[]>;
Copy link
Collaborator

@LucasLefevre LucasLefevre Dec 30, 2025

Choose a reason for hiding this comment

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

nitpicking to make it shorter, we know we are in the context of pivots

Suggested change
pivotMeasureDependencies: Record<UID, Range[]>;
measureDependencies: Record<UID, Range[]>;

src/plugins/core/pivot.ts Outdated
for (const measure of measures) {
if (measure.computedBy) {
const compiledFormula = this.getMeasureCompiledFormula(pivotId, measure);
ranges.push(...compiledFormula.dependencies);
Copy link
Collaborator

@LucasLefevre LucasLefevre Dec 30, 2025

Choose a reason for hiding this comment

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

this could be mixed in the for loop just above

src/functions/helper_lookup.ts
if (measure.computedBy) {
const formula = evalContext.getters.getMeasureCompiledFormula(measure);
dependencies.push(...formula.dependencies.filter((range) => !range.invalidXc));
dependencies.push(...evalContext.getters.getMeasureFullDependencies(pivotId, measure));
Copy link
Collaborator

@LucasLefevre LucasLefevre Dec 30, 2025

Choose a reason for hiding this comment

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

Since the new approach where we get all dependencies, this adds the same dependencies multiple times.

fdamhaut and others added 2 commits December 31, 2025 09:44
@fdamhaut @rrahir
[FIX] demo: add import osheet
434f1b0 
Allow to upload json files to ease development/bug investigation

closes #7591

Task: 0
Signed-off-by: Rémi Rahir (rar) <[email protected]>
@rrahir
[FIX] Pivots: Recompute measure on indirect dependency update
4dd170f 
Task: 5349782
@rrahir rrahir force-pushed the 18.0-pivot-measure-indirect-dependencies-rar branch from f7a3cfc to e8f6a26 Compare December 31, 2025 08:44
@rrahir rrahir force-pushed the 18.0-pivot-measure-indirect-dependencies-rar branch 3 times, most recently from 49e8d82 to b70d71d Compare January 5, 2026 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LucasLefevre LucasLefevre LucasLefevre left review comments

@hokolomopo hokolomopo hokolomopo left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rrahir @robodoo @LucasLefevre @hokolomopo @fdamhaut