-
Notifications
You must be signed in to change notification settings - Fork 398
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
Aggregate multiple columns behavior & parameter list support #1702
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
e6e3f08
to
cd77e91
Compare
cd77e91
to
f33f2d2
Compare
There was a problem hiding this 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.
src/Propel/Generator/Behavior/AggregateMultipleColumns/AggregateMultipleColumnsBehavior.php
Outdated
Show resolved
Hide resolved
src/Propel/Generator/Behavior/AggregateMultipleColumns/AggregateMultipleColumnsBehavior.php
Outdated
Show resolved
Hide resolved
src/Propel/Generator/Behavior/AggregateMultipleColumns/AggregateMultipleColumnsBehavior.php
Outdated
Show resolved
Hide resolved
src/Propel/Generator/Behavior/AggregateMultipleColumns/AggregateMultipleColumnsBehavior.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
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? |
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:
If you have feedback, I am happy to hear it!
Thank you, best wishes