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.

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

Aggregate multiple columns behavior & parameter list support #1702

Merged

Conversation

mringler
Copy link
Contributor

In a project, I have to propagate several aggregated columns across several tables using the AggregateColumn behavior. It is similar to having items, which are aggregated on their groups, which are aggregated on their owner. This works fine, except I noticed that the AggregateColumn behavior causes extensive update cascades when changing the aggregation source table. In my project, there are four aggregation columns on three tables, and a single insert, update or delete on the source table causes a whooping 32 subsequent updates. This has become a problem, particularly when deleting several items takes about a minute, when it should be a fraction of a second.
Reason for the update cascade is that AggregateColumn behavior triggers a save for each aggregation, which in turn triggers the full aggregation on the parent table, and so on, leading to exponential complexity.

I figured there is an easy solution for this: just have AggregateColumn do all the aggregations and only then trigger save, and we are back at linear complexity. In my project, that means 3 instead of 32 updates, and easy adjustment in case of delete. So that's fantastic, and easy to implement using Propel behavior.
However, I need Propel schema definition to supply a list of columns to the behavior, and this is currently not supported. Even if I wrote my own little behavior, the only way to get it working is with some funky business, like putting json into an xml parameter attribute. Bummer. If only there were parameter list support in the schema parser...

Long story short, I figured I can do it in Propel itself, for the sake of the blazing fast of everybody. Seems to work nicely, all the tests run through. Hope you can use it, so I don't have to monkey patch around the issue.

For reference, the behavior can be declared like this:

<behavior name="aggregate_multiple_columns" id="...">
    <parameter name="foreign_table" value="score"/>
    <parameter-list name="columns">
        <parameter-list-item>
            <parameter name="column_name" value="total_score" />
            <parameter name="expression" value="SUM(score)" />
        </parameter-list-item>
        <parameter-list-item>
            <parameter name="column_name" value="number_of_scores" />
            <parameter name="expression" value="COUNT(score)" />
        </parameter-list-item>
    </parameter-list>
</behavior>

If you have feedback, I am happy to hear it!
Thank you, best wishes

@codecov-io
Copy link

codecov-io commented Feb 26, 2021

Codecov Report

Merging #1702 (cd77e91) into master (394a6ce) will increase coverage by 0.09%.
The diff coverage is 96.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1702      +/-   ##
============================================
+ Coverage     88.16%   88.26%   +0.09%     
- Complexity     7656     7706      +50     
============================================
  Files           258      261       +3     
  Lines         21893    22081     +188     
============================================
+ Hits          19303    19490     +187     
- Misses         2590     2591       +1     
Flag Coverage Δ Complexity Δ
5-max 88.26% <96.33%> (+0.09%) 0.00 <113.00> (ø)
7.4 88.26% <96.33%> (+0.09%) 0.00 <113.00> (ø)
agnostic 73.53% <91.74%> (+0.17%) 0.00 <113.00> (ø)
mysql 71.52% <88.53%> (+1.80%) 0.00 <113.00> (ø)
pgsql 70.45% <88.07%> (+0.20%) 0.00 <113.00> (ø)
sqlite ? ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ltipleColumns/AggregateMultipleColumnsBehavior.php 96.06% <96.06%> (ø) 33.00 <33.00> (?)
src/Propel/Generator/Builder/Util/SchemaReader.php 91.28% <96.15%> (+4.81%) 92.00 <80.00> (+17.00)
...gregateMultipleColumns/templates/objectCompute.php 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...ggregateMultipleColumns/templates/objectUpdate.php 100.00% <100.00%> (ø) 0.00 <0.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 394a6ce...cd77e91. Read the comment docs.

@mringler mringler force-pushed the aggregate_multiple_columns_behavior branch 3 times, most recently from e6e3f08 to cd77e91 Compare February 27, 2021 08:19
@mringler mringler force-pushed the aggregate_multiple_columns_behavior branch from cd77e91 to f33f2d2 Compare March 3, 2021 18:29
Copy link
Contributor

@stereomon stereomon left a comment

Choose a reason for hiding this comment

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

Only a couple small things that I would like to see updated. Please also make sure that your code is on top of the latest master.

Copy link
Contributor

@stereomon stereomon left a comment

Choose a reason for hiding this comment

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

Looks good to me except that the tests could also make use of return-types but that's not a blocker for me.

When checks are green we can merge this.

@mringler
Copy link
Contributor Author

mringler commented Mar 4, 2021

Looks good to me except that the tests could also make use of return-types but that's not a blocker for me.

When checks are green we can merge this.

Great! I have linted the tests and added return types, too. I would like to squash the commits and force-push, will I lose your review then?

@dereuromark dereuromark merged commit 399c82f into propelorm:master Mar 4, 2021
@mringler mringler deleted the aggregate_multiple_columns_behavior branch September 29, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants