CRM-20199 Contribution search performance #9631

eileenmcnaughton · 2017-01-04T06:39:30Z

eileenmcnaughton · 2017-01-04T06:52:56Z

@colemanw @monishdeb FYI - I am trying to clean up the contribution search class a bit in order to fix some of the performance issues. I dug into why some fields were manually declared in the hope of being a bit more meta-data-ey & found that it was because they were set to export = false. In each case a whole lot of handling had been added to compensate for it so I think we can add export = true more aggressively.

I'd like to remove the handling for contribution_check_number but it seems that as a result of it being there contribution_check_number is accepted & output (which is used by search fields & could possibly conflict with trxn_id I guess.....). I think the hated uniquenames are the right answer here + add a bit of handling in the contribution.get api to copy $result['contribution_check_number' to $result['check_number'] - this doesn't happen currently for contribution_source but perhaps it should.

That + dealing properly with the pseudoconstant fields (payment_instrument, financial_type, contribution_status is next)

Contribution is of course one of a handful of api using the query object....

eileenmcnaughton · 2017-01-04T06:53:22Z

CRM/Contribute/BAO/Query.php

@@ -157,100 +151,12 @@ public static function select(&$query) {
      $query->_tables['civicrm_contribution'] = 1;
    }

-    // LCD 716
-    $includeSoftCredits = self::isSoftCreditOptionEnabled($query->_params);


This is just moved

eileenmcnaughton · 2017-01-04T06:53:35Z

CRM/Contribute/BAO/Query.php

    if (!empty($query->_returnProperties['contribution_campaign_title'])) {
      $query->_select['contribution_campaign_title'] = "civicrm_campaign.title as contribution_campaign_title";
      $query->_element['contribution_campaign_title'] = $query->_tables['civicrm_campaign'] = 1;
    }

-    // Adding address_id in a way that is more easily extendable since the above is a bit ... wordy.
-    $supportedBasicReturnValues = array('address_id');


not needed now the field is set to export

eileenmcnaughton · 2017-01-04T06:54:07Z

CRM/Contribute/BAO/Query.php

-    }
-
-    //CRM-16116: get financial_type_id
-    if (!empty($query->_returnProperties['financial_type_id'])) {


not needed now the field is set to export

monishdeb · 2017-01-04T13:55:29Z

Jenkin test this please

eileenmcnaughton · 2017-01-05T00:31:04Z

Phew, I got it to pass. I do think handling oddness is better done near the 'edges' of the app - ie. forms & apis can handle field wierdness & pushing wierdness out from the BAO is generally good

eileenmcnaughton · 2017-01-10T11:10:14Z

CRM/Contact/BAO/Query.php

      if (
        (substr($name, 0, 12) == 'participant_') ||
        (substr($name, 0, 7) == 'pledge_') ||
-        (substr($name, 0, 5) == 'case_') ||


ie do not skip the loop for fields starting with contribution_ as many of them are in fact better dealt with in that context since they can be derived from metadata

eileenmcnaughton · 2017-01-10T11:12:19Z

CRM/Contact/BAO/Query.php

@@ -680,15 +697,12 @@ public function selectClause($apiEntity = NULL) {
        }
      }

-      if (in_array($name, array('prefix_id', 'suffix_id', 'gender_id', 'communication_style_id'))) {


Consolidate this logic with general option-value pseudoconstant metadata based evaluation further down

eileenmcnaughton · 2017-01-10T11:12:41Z

CRM/Contact/BAO/Query.php

+      if (
+        !empty($this->_paramLookup[$name])
+        || !empty($this->_returnProperties[$name])
+        || $this->pseudoConstantNameIsInReturnProperties($field)


$this->pseudoConstantNameIsInReturnProperties replaces the earlier in_array call

eileenmcnaughton · 2017-01-10T11:13:23Z

CRM/Contact/BAO/Query.php

-                if ($fieldName == 'organization_name') {
-                  $this->_select[$name] = "IF ( contact_a.contact_type = 'Individual', NULL, contact_a.organization_name ) as organization_name";
-                }
-                elseif ($fieldName != 'id') {


move all this stuff out of the 'contact-table-specific' & make it just 'metadata-based'

eileenmcnaughton · 2017-01-10T11:14:56Z

CRM/Contact/BAO/Query.php

+                // If we have an option group defined then rather than joining the option value table in
+                // (which is an unindexed join) we render the option value on output.
+                // @todo - extend this to other pseudoconstants.
+                if ($this->pseudoConstantNameIsInReturnProperties($field)) {


By adding all option-value based fields (that get here) to the pseudoconstant array they become workable in the context of the $convertToPseudoFields fn

eileenmcnaughton · 2017-01-10T11:19:18Z

CRM/Contact/BAO/Query.php

      }
      else {
        $order = " ORDER BY contact_a.sort_name asc, contact_a.id";
      }
    }
+    if (!$order && empty($orderByArray)) {


Refactoring in this function ensures an array of order-by fields although it is still pretty funky. Previously the function did not iterate through all fields - just the first one in the string, to add special handling.

The handling we are adding is a way of avoiding joining the option_value table in when sorting by it. By rendering the values in php & using field($fieldName, 4,6,7) I was able to render the query without the join. Results in test were 10 times faster, in some cases much more.

eileenmcnaughton · 2017-01-10T11:20:48Z

CRM/Contribute/BAO/Contribution.php

@@ -825,15 +838,6 @@ public static function &exportableFields($checkPermission = TRUE) {
        ),
      );

-      $contributionRecurId = array(


As part of this PR I declared this field as exportable so it no longer needs manual adding to exportable fields

eileenmcnaughton · 2017-01-10T11:21:20Z

CRM/Contribute/BAO/Contribution.php

      $expFieldProduct = CRM_Contribute_DAO_Product::export();
      $expFieldsContrib = CRM_Contribute_DAO_ContributionProduct::export();
      $typeField = CRM_Financial_DAO_FinancialType::export();
      $financialAccount = CRM_Financial_DAO_FinancialAccount::export();
-      $optionField = CRM_Core_OptionValue::getFields($mode = 'contribute');


as part of this PR I declared payment_instrument_id as exportable so it no longer needs manual adding

eileenmcnaughton · 2017-01-10T11:21:46Z

CRM/Contribute/BAO/Contribution.php

      $expFieldProduct = CRM_Contribute_DAO_Product::export();
      $expFieldsContrib = CRM_Contribute_DAO_ContributionProduct::export();
      $typeField = CRM_Financial_DAO_FinancialType::export();
      $financialAccount = CRM_Financial_DAO_FinancialAccount::export();
-      $optionField = CRM_Core_OptionValue::getFields($mode = 'contribute');
-      $contributionStatus = array(


now handled by the pseudoconstant - not needed as a manual add on

…ion search. This involves setting payment instrument to exportable & remove hacks that were used in place of doing that, and adding new logic to the order by to allow it to order without joining the option value table (using the field() function). I have observed one small regression which is the Qill showing 'Payment Method ID' instead of payment method. I'm committed to fixing that but I would push to merge this in advance of that because we are in the best place in the monthly cycle to merge this and that is a very minor fallout.

eileenmcnaughton · 2017-03-03T05:37:53Z

@seamuslee001 this is the remaining work to remove the slow joins. I don't feel I can break it into any smaller chunks & I do think this is a good time of the cycle to merge it.

From the commit notes
This involves setting payment instrument to exportable & remove hacks that were used in
place of doing that, and adding new logic to the order by to allow it to order without
joining the option value table (using the field() function).

I have observed one small regression which is the Qill showing 'Payment Method ID'
instead of 'Payment Method'. I'm committed to fixing that but I would push
to merge this in advance of that because we are in the best place in the monthly
cycle to merge this and that is a very minor fallout.

seamuslee001 · 2017-03-07T21:24:23Z

@eileenmcnaughton i have tested this on export and search and all appears fine. I think we can address the Qill issue in another PR

seamuslee001 · 2017-03-07T21:44:11Z

I did an export of a saved field mapping, There were no signs of fields missing and then on export fields were correctly populated as expected

eileenmcnaughton · 2017-03-07T22:04:28Z

:-)

…arch CRM-20199 Contribution search performance

civicrm-builder added the master label Jan 4, 2017

eileenmcnaughton commented Jan 4, 2017

View reviewed changes

eileenmcnaughton mentioned this pull request Jan 4, 2017

wip CRM-19815 contribution search performance fixes #9604

Closed

eileenmcnaughton force-pushed the contribution_search branch 3 times, most recently from c582a7d to 38af28f Compare January 4, 2017 23:50

eileenmcnaughton force-pushed the contribution_search branch 10 times, most recently from 7a18987 to bcd9856 Compare January 10, 2017 10:24

eileenmcnaughton commented Jan 10, 2017

View reviewed changes

eileenmcnaughton mentioned this pull request Jan 24, 2017

CRM-19830 CRM-19815 clean up on contribution search #9718

Merged

eileenmcnaughton force-pushed the contribution_search branch 3 times, most recently from 0b8429f to 74ec330 Compare January 26, 2017 11:48

eileenmcnaughton force-pushed the contribution_search branch 10 times, most recently from ba3db65 to e880e69 Compare February 7, 2017 17:39

eileenmcnaughton force-pushed the contribution_search branch 2 times, most recently from ef71754 to 3cbea72 Compare February 23, 2017 03:39

eileenmcnaughton mentioned this pull request Feb 23, 2017

Towards CRM-19815, CRM-19830 make pseudoconstant handling more generic in order to improve performance #9885

Merged

eileenmcnaughton force-pushed the contribution_search branch 3 times, most recently from 25c03dd to 4bd03eb Compare March 1, 2017 21:30

eileenmcnaughton force-pushed the contribution_search branch from 4bd03eb to 9d5c7f1 Compare March 3, 2017 05:32

eileenmcnaughton changed the title ~~CRM-19815 Contribution search performance & CRM-19830 Contribution search de-crufting~~ CRM-20199 Contribution search performance Mar 3, 2017

eileenmcnaughton merged commit b733a3a into civicrm:master Mar 7, 2017

eileenmcnaughton deleted the contribution_search branch March 7, 2017 21:29

monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017

Merge pull request civicrm#9631 from eileenmcnaughton/contribution_se…

a1122c5

…arch CRM-20199 Contribution search performance

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-20199 Contribution search performance #9631

CRM-20199 Contribution search performance #9631

eileenmcnaughton commented Jan 4, 2017 •

edited by civicrm-builder

Loading

eileenmcnaughton commented Jan 4, 2017

eileenmcnaughton Jan 4, 2017

eileenmcnaughton Jan 4, 2017 •

edited

Loading

eileenmcnaughton Jan 4, 2017

monishdeb commented Jan 4, 2017

eileenmcnaughton commented Jan 5, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton Jan 10, 2017

eileenmcnaughton commented Mar 3, 2017

seamuslee001 commented Mar 7, 2017

seamuslee001 commented Mar 7, 2017

eileenmcnaughton commented Mar 7, 2017

CRM-20199 Contribution search performance #9631

CRM-20199 Contribution search performance #9631

Conversation

eileenmcnaughton commented Jan 4, 2017 • edited by civicrm-builder Loading

eileenmcnaughton commented Jan 4, 2017

Choose a reason for hiding this comment

eileenmcnaughton Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented Jan 4, 2017

eileenmcnaughton commented Jan 5, 2017

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

eileenmcnaughton commented Mar 3, 2017

seamuslee001 commented Mar 7, 2017

seamuslee001 commented Mar 7, 2017

eileenmcnaughton commented Mar 7, 2017

eileenmcnaughton commented Jan 4, 2017 •

edited by civicrm-builder

Loading

eileenmcnaughton Jan 4, 2017 •

edited

Loading