Extract query to generate annual totals and add test #12810

eileenmcnaughton · 2018-09-12T05:34:46Z

Overview

This PR extracts the function that generates the annual totals & adds a test.

Before

No test

After

Test exists both for with & without financial type ACLS. One unnecessary clause is removed from non-ACL sites

Technical Details

If the PR introduces noteworthy technical changes, please describe them here. Provide code snippets if necessary

Comments

This is primarily a preliminary cleanup. We are seeing contribution tabs on our contact with the most contributions (18k) take 6 seconds on this query. From my tests the reason is a combination of

unnecessary filter on financial_type_id (given no financial acls are enabled)
unecessary join on civicrm_line_item (given no financial acls are enabled)
poor index selection between contact_id & status_id - explain shows

|    1 | SIMPLE      | b     | index_merge | index_contribution_status,FK_civicrm_contribution_contact_id | FK_civicrm_contribution_contact_id,index_contribution_status | 4,5     | NULL | 17649 | Using intersect(FK_civicrm_contribution_contact_id,index_contribution_status); Using where |
``

I'm going to follow up with a PR that makes more performance change - this is primarily preliminary to do the refactor first.

eileenmcnaughton · 2018-09-12T05:52:02Z

Performance discussion - the below should not prevent this being merged - it is about the next steps

@monishdeb @lcdservices @seamuslee001 @pradpnayak @davejenx I'm interested to see someone else's performance testing results - I realise our Db is a bit of an anomaly & it's likely no-one else has a single contact with 18k donations or however many million contributions we have. However, even confirmation that changing the query to something that looks like the last one in the comment below 'does no harm' would be good.

Note that the last one above is what would happen for sites WITH financial acls enabled. There are 2 significant changes to the query

contribution status id clause moved to an outer query - this was the one that made most of the difference as it started making better key choices. This query is not used for vast numbers of contacts - in fact I believe it's only ever one contact - so honing in on the contact first is good.
changing the line item filter to being in a subquery - this means I can restructure it to either use the (recommended) selectWhereClause hook or use a function that follows the same format with a goal to move to the selectWhereClause hook down the track.

As an aside I am not fully understanding the line item subquery - ie. the use of NOT IN - see the very first query above

Current query

   SELECT count(*) as count,
             sum(total_amount) as amount,
             avg(total_amount) as average,
             currency
      FROM civicrm_contribution b
      LEFT JOIN civicrm_line_item i ON i.contribution_id = b.id AND i.entity_table = 'civicrm_contribution'  AND i.financial_type_id NOT IN (9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,26)
      WHERE b.contact_id IN ( 72 ) AND b.contribution_status_id = 1 AND b.is_test = 0 AND b.receive_date >= 20180101 AND b.receive_date <  20190101 AND b.contribution_status_id = 1 AND  b.financial_type_id IN (9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,26)
      GROUP BY currency;

6 seconds 20

Without unnecessary ACL clauses

     SELECT count(*) as count,
             sum(total_amount) as amount,
             avg(total_amount) as average,
             currency
      FROM civicrm_contribution b

      WHERE b.contact_id IN ( 72 ) AND b.contribution_status_id = 1 AND b.is_test = 0 AND b.receive_date >= 20180101 AND b.receive_date <  20190101 AND b.contribution_status_id = 1      GROUP BY currency;

6 seconds 20

Restructured for performance

      SELECT count(*) as count,
             sum(total_amount) as amount,
             avg(total_amount) as average,
             currency
      FROM 
      (SELECT total_amount, currency, contribution_status_id, receive_date FROM
      civicrm_contribution b

      WHERE b.contact_id IN ( 72 ) AND b.is_test = 0 
      ) as conts
       WHERE conts.receive_date >= 20180101 AND conts.receive_date <  20190101
           GROUP BY currency, contribution_status_id;

0.06 seconds

Restructured for performance (with extra Having)

    SELECT count(*) as count,
             sum(total_amount) as amount,
             avg(total_amount) as average,
             currency
      FROM 
      (SELECT total_amount, currency, contribution_status_id, receive_date FROM
      civicrm_contribution b

      WHERE b.contact_id IN ( 72 ) AND b.is_test = 0 
      ) as conts
       WHERE conts.receive_date >= 20180101 AND conts.receive_date <  20190101
           GROUP BY currency, contribution_status_id
           HAVING contribution_status_id = 1;

0.05 seconds

Restructured for performance (with extra Having) & financial types back

    SELECT count(*) as count,
             sum(total_amount) as amount,
             avg(total_amount) as average,
             currency
      FROM 
      (SELECT total_amount, currency, contribution_status_id, receive_date 
      FROM
      civicrm_contribution b

      WHERE b.contact_id IN ( 72 ) AND b.is_test = 0 AND b.id IN (SELECT c.id FROM civicrm_contribution c INNER JOIN civicrm_line_item li ON c.id = li.entity_id  AND li.entity_table = 'civicrm_contribution' AND li.financial_type_id IN (9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,26)
) AND b.financial_type_id IN (9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,26)
      ) as conts
       WHERE conts.receive_date >= 20180101 AND conts.receive_date <  20190101
           GROUP BY currency, contribution_status_id
           HAVING contribution_status_id = 1;

.07 seconds

Proposed final query Restructured for performance (with extra Having) & financial types back for ACL enabled sites

SELECT SQL_NO_CACHE count(*) as count,
            sum(total_amount) as amount,
          avg(total_amount) as average,
            currency
     FROM 
     (SELECT total_amount, currency, contribution_status_id, receive_date 
    FROM
     civicrm_contribution b

 WHERE b.contact_id IN ( 72 ) AND b.is_test = 0 AND b.id IN (SELECT c.id FROM civicrm_contribution c INNER JOIN civicrm_line_item li ON c.id = li.entity_id  AND li.entity_table = 'civicrm_contribution' AND li.financial_type_id IN (9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,26)
)
 ) as conts
 WHERE conts.receive_date >= 20180101 AND conts.receive_date <  20190101
 GROUP BY currency, contribution_status_id
HAVING contribution_status_id = 1;

0.09 seconds

JoeMurray · 2018-09-17T14:25:23Z

@monishdeb we have an extension that provides financial acls for reports. It was created after we factored out this functionality from core due to performance reasons. We should a) ensure that there is a message displayed when enabling financial acl in core that the extension is required (I'm not sure that we did this) and b) see if some of the performance refactoring here might be useful in the extension.

eileenmcnaughton · 2018-10-07T11:36:25Z

test this please

JohnFF

Minor feedback around standards and clarity. Otherwise looks good.

JohnFF · 2018-10-10T09:53:34Z

CRM/Contribute/BAO/Contribution.php

+      'b.is_test = 0',
+      'b.receive_date >= ' . $startDate,
+      'b.receive_date <  ' . $endDate,
+      'b.contribution_status_id = 1',


Don't need b.contribution_status_id = 1 in there twice (I appreciate it is important).

Far more important, it should not be 1. 1 isn't clear. I don't know what 1 maps to - a Constant or PseudoConstant would be better here.

JohnFF · 2018-10-10T09:54:18Z

CRM/Contribute/BAO/Contribution.php

+      $monthDay = $config->fiscalYearStart['M'] . $config->fiscalYearStart['d'];
+    }
+    else {
+      $monthDay = '0101';


A comment like "First of January" at the end of the 0101 line would help.

JohnFF · 2018-10-10T09:55:03Z

CRM/Contribute/BAO/Contribution.php

+   *
+   * @return string
+   */
+  public static function getAnnualQuery($contactIDs) {


Some detail in the comment would help. What does this function do? I know it returns a query, but for what? etc.

JohnFF · 2018-10-10T09:59:41Z

CRM/Contribute/BAO/Contribution.php

+    $config = CRM_Core_Config::singleton();
+    $currentMonth = date('m');
+    $currentDay = date('d');
+    if ((int ) $config->fiscalYearStart['M'] > $currentMonth ||


Should be (int).

JohnFF · 2018-10-10T10:00:17Z

CRM/Contribute/BAO/Contribution.php

+    $currentDay = date('d');
+    if ((int ) $config->fiscalYearStart['M'] > $currentMonth ||
+      ((int ) $config->fiscalYearStart['M'] == $currentMonth &&
+        (int ) $config->fiscalYearStart['d'] > $currentDay


Please bracket this - I know the order of execution but it can be confusing.

I changed the line breaks which I think makes it clear

JohnFF · 2018-10-10T10:02:47Z

CRM/Contribute/BAO/Contribution.php

+    $endDate = "$nextYear$monthDay";
+    $financialTypes = [];
+    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
+    $liWhere = " AND i.financial_type_id IN (0)";


What is the significance of 0?

comment / todo added

JohnFF · 2018-10-10T10:04:05Z

CRM/Contribute/BAO/Contribution.php

+    CRM_Financial_BAO_FinancialType::buildPermissionedClause($whereClauses, NULL, 'b');
+    $query = "
+      SELECT count(*) as count,
+             sum(total_amount) as amount,


COUNT, SUM, AVG AS should probably be all in caps (I think is the convention)

JohnFF · 2018-10-10T10:05:52Z

CRM/Contribute/BAO/Contribution.php

+        $newFiscalYearStart['M'] = '0' . $newFiscalYearStart['M'];
+      }
+      if ($newFiscalYearStart['d'] < 10) {
+        $newFiscalYearStart['d'] = '0' . $newFiscalYearStart['d'];


Presumably this is to convert 1 into 01. I'm 50/50 on whether or a comment should say that's what this is doing.

comment / todo added

JohnFF · 2018-10-10T10:07:26Z

tests/phpunit/CRM/Contribute/BAO/ContributionTest.php

+  public function testAnnualQueryWithFinancialACLsDisabled() {
+    $sql = CRM_Contribute_BAO_Contribution::getAnnualQuery([1, 2, 3]);
+    $this->assertContains('sum(total_amount) as amount,', $sql);
+    $this->assertContains('WHERE b.contact_id IN ( 1,2,3 )', $sql);


I think this should be IN (1, 2, 3)

JohnFF · 2018-10-10T10:09:04Z

CRM/Contribute/BAO/Contribution.php

@@ -1316,68 +1316,11 @@ public static function sortName($id) {
   * @return array


Seeing as we're in the headspace - please could you add a helpful comment into annual function's header? What is annual? What does it do?

seamuslee001 · 2018-10-10T21:30:41Z

@eileenmcnaughton test fail looks related (noting i think it might just be a correction to the test - whitespace issue

@JoeMurray Joe there is this

civicrm-core/CRM/Upgrade/Incremental/General.php

Line 113 in 6f4f6c8

    
           $preUpgradeMessage .= '<br />' . ts('CiviCRM will in the future require the extension %1 for CiviCRM Reports to work correctly with the Financial Type ACLs. The extension can be downloaded <a href="%2">here</a>', array(

which may be something like what your thinking off, note that that is only shown on upgrade or maybe its a different extension all together

JoeMurray · 2018-10-10T22:20:36Z

Yeah, that would be a message along the lines I described. Thx.

eileenmcnaughton force-pushed the annual branch from c597801 to 1cbcdb0 Compare September 12, 2018 05:53

seamuslee001 added the master label Sep 13, 2018

JohnFF suggested changes Oct 10, 2018

View reviewed changes

eileenmcnaughton force-pushed the annual branch from 1cbcdb0 to d92597e Compare October 10, 2018 10:37

JohnFF approved these changes Oct 10, 2018

View reviewed changes

eileenmcnaughton force-pushed the annual branch from d92597e to 55187b2 Compare October 10, 2018 11:14

eileenmcnaughton added the merge on pass label Oct 10, 2018

eileenmcnaughton force-pushed the annual branch from 55187b2 to 44267a4 Compare October 10, 2018 13:00

eileenmcnaughton force-pushed the annual branch from 44267a4 to f09517b Compare October 11, 2018 10:07

Extract query to generate annual totals and add test

6b60d32

eileenmcnaughton force-pushed the annual branch from f09517b to 6b60d32 Compare October 11, 2018 10:23

eileenmcnaughton merged commit 314c553 into civicrm:master Oct 11, 2018

eileenmcnaughton deleted the annual branch October 11, 2018 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract query to generate annual totals and add test #12810

Extract query to generate annual totals and add test #12810

eileenmcnaughton commented Sep 12, 2018

eileenmcnaughton commented Sep 12, 2018 •

edited

Loading

JoeMurray commented Sep 17, 2018

eileenmcnaughton commented Oct 7, 2018

JohnFF left a comment

JohnFF Oct 10, 2018 •

edited

Loading

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018 •

edited

Loading

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018

JohnFF Oct 10, 2018

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018

eileenmcnaughton Oct 10, 2018

JohnFF Oct 10, 2018

eileenmcnaughton Oct 10, 2018

seamuslee001 commented Oct 10, 2018

JoeMurray commented Oct 10, 2018

		@@ -1316,68 +1316,11 @@ public static function sortName($id) {
		* @return array

Extract query to generate annual totals and add test #12810

Extract query to generate annual totals and add test #12810

Conversation

eileenmcnaughton commented Sep 12, 2018

Overview

Before

After

Technical Details

Comments

eileenmcnaughton commented Sep 12, 2018 • edited Loading

JoeMurray commented Sep 17, 2018

eileenmcnaughton commented Oct 7, 2018

JohnFF left a comment

Choose a reason for hiding this comment

JohnFF Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnFF Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Oct 10, 2018

JoeMurray commented Oct 10, 2018

eileenmcnaughton commented Sep 12, 2018 •

edited

Loading

JohnFF Oct 10, 2018 •

edited

Loading

JohnFF Oct 10, 2018 •

edited

Loading