From 124f3c1b17c5465b55a0c57230e0c0cc95f5ddba Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 28 Feb 2019 16:55:21 +1300 Subject: [PATCH] Eliminate unnecessary join to civicrm_financial_type table to improve performance Although this join is indexed it affects index choice, often causing the right index not to be used and slowing performance. I got subtantial speed improvement by eliminating this join --- CRM/Contact/BAO/Query.php | 107 ++++++++++++++---- CRM/Contribute/BAO/Query.php | 8 -- .../phpunit/CRM/Contribute/BAO/QueryTest.php | 25 ++-- tests/phpunit/CRM/Export/BAO/ExportTest.php | 2 +- 4 files changed, 100 insertions(+), 42 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index d30c4bbaab..be480e7e77 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -729,7 +729,7 @@ class CRM_Contact_BAO_Query { if ( !empty($this->_paramLookup[$name]) || !empty($this->_returnProperties[$name]) - || $this->pseudoConstantNameIsInReturnProperties($field) + || $this->pseudoConstantNameIsInReturnProperties($field, $name) || $makeException ) { if ($cfID) { @@ -870,10 +870,10 @@ class CRM_Contact_BAO_Query { $this->_select[$name] = "{$field['where']} as `$name`"; } } + elseif ($this->pseudoConstantNameIsInReturnProperties($field, $name)) { + $this->addPseudoconstantFieldToSelect($name); + } else { - if ($this->pseudoConstantNameIsInReturnProperties($field, $name)) { - $this->addPseudoconstantFieldToSelect($field, $name, $tableName); - } $this->_select[$name] = str_replace('civicrm_contact.', 'contact_a.', "{$field['where']} as `$name`"); } if (!in_array($tName, array('state_province', 'country', 'county'))) { @@ -6339,7 +6339,8 @@ AND displayRelType.is_active = 1 // ORDER BY field('contribution_status_id', 2,1,4) // we can remove a join. In the case of the option value join it is /// a join known to cause slow queries. - // @todo cover other pseudoconstant types. Limited to option group ones in the + // @todo cover other pseudoconstant types. Limited to option group ones & Foreign keys + // matching an id+name parrern in the // first instance for scope reasons. They require slightly different handling as the column (label) // is not declared for them. // @todo so far only integer fields are being handled. If we add string fields we need to look at @@ -6348,11 +6349,17 @@ AND displayRelType.is_active = 1 $pseudoConstantMetadata = CRM_Utils_Array::value('pseudoconstant', $fieldSpec, FALSE); if (!empty($pseudoConstantMetadata) ) { - if (!empty($pseudoConstantMetadata['optionGroupName'])) { + if (!empty($pseudoConstantMetadata['optionGroupName']) + || $this->isPseudoFieldAnFK($fieldSpec) + ) { $sortedOptions = $fieldSpec['bao']::buildOptions($fieldSpec['name'], NULL, [ - 'orderColumn' => 'label', + 'orderColumn' => CRM_Utils_Array::value('labelColumn', $pseudoConstantMetadata, 'label'), ]); - $order = str_replace("$field", "field({$fieldSpec['name']}," . implode(',', array_keys($sortedOptions)) . ")", $order); + $fieldIDsInOrder = implode(',', array_keys($sortedOptions)); + // Pretty sure this validation ALSO happens in the order clause & this can't be reached but... + // this might give some early warning. + CRM_Utils_Type::validate($fieldIDsInOrder, 'CommaSeparatedIntegers'); + $order = str_replace("$field", "field({$fieldSpec['name']},$fieldIDsInOrder)", $order); } //CRM-12565 add "`" around $field if it is a pseudo constant // This appears to be for 'special' fields like locations with appended numbers or hyphens .. maybe. @@ -6433,11 +6440,19 @@ AND displayRelType.is_active = 1 * @return bool */ private function pseudoConstantNameIsInReturnProperties($field, $fieldName = NULL) { - if (!isset($field['pseudoconstant']['optionGroupName'])) { + $realField = $this->getMetadataForRealField($fieldName); + if (!isset($realField['pseudoconstant'])) { + return FALSE; + } + $pseudoConstant = $realField['pseudoconstant']; + if (empty($pseudoConstant['optionGroupName']) && + CRM_Utils_Array::value('labelColumn', $pseudoConstant) !== 'name') { + // We are increasing our pseudoconstant handling - but still very cautiously, + // hence the check for labelColumn === name return FALSE; } - if (CRM_Utils_Array::value($field['pseudoconstant']['optionGroupName'], $this->_returnProperties)) { + if (!empty($pseudoConstant['optionGroupName']) && CRM_Utils_Array::value($pseudoConstant['optionGroupName'], $this->_returnProperties)) { return TRUE; } if (CRM_Utils_Array::value($fieldName, $this->_returnProperties)) { @@ -6739,7 +6754,7 @@ AND displayRelType.is_active = 1 if (!empty($field) && empty($field['name'])) { // standardising field formatting here - over time we can phase out variants. // all paths using this currently unit tested - $field['name'] = CRM_Utils_Array::value('field_name', $field, $field['idCol']); + $field['name'] = CRM_Utils_Array::value('field_name', $field, CRM_Utils_Array::value('idCol', $field, $fieldName)); } return $field; } @@ -6753,7 +6768,24 @@ AND displayRelType.is_active = 1 */ protected function getMetadataForRealField($fieldName) { $field = $this->getMetadataForField($fieldName); - return empty($field['is_pseudofield_for']) ? $field : $this->getMetadataForField($field['is_pseudofield_for']); + if (!empty($field['is_pseudofield_for'])) { + $field = $this->getMetadataForField($field['is_pseudofield_for']); + $field['pseudofield_name'] = $fieldName; + } + elseif (!empty($field['pseudoconstant'])) { + if (!empty($field['pseudoconstant']['optionGroupName'])) { + $field['pseudofield_name'] = $field['pseudoconstant']['optionGroupName']; + if (empty($field['table_name'])) { + if (!empty($field['where'])) { + $field['table_name'] = explode('.', $field['where'])[0]; + } + else { + $field['table_name'] = 'civicrm_contact'; + } + } + } + } + return $field; } /** @@ -6764,21 +6796,46 @@ AND displayRelType.is_active = 1 * @todo - so far this applies to a narrow range of pseudocontants. We are adding them * carefully with test coverage but aim to extend. * - * @param array $field * @param string $name - * @param string $tableName */ - protected function addPseudoconstantFieldToSelect($field, $name, $tableName) { - $pseudoFieldName = $field['pseudoconstant']['optionGroupName']; - $this->_pseudoConstantsSelect[$pseudoFieldName] = [ - 'pseudoField' => $field['name'], - 'idCol' => $name, - 'field_name' => $field['name'], - 'bao' => $field['bao'], - 'pseudoconstant' => $field['pseudoconstant'], - ]; - $this->_tables[$tableName] = 1; - $this->_element[$pseudoFieldName] = 1; + protected function addPseudoconstantFieldToSelect($name) { + $field = $this->getMetadataForRealField($name); + $realFieldName = $field['name']; + $pseudoFieldName = CRM_Utils_Array::value('pseudofield_name', $field); + if ($pseudoFieldName) { + // @todo - we don't really need to build this array now we have metadata more available with getMetadataForField fn. + $this->_pseudoConstantsSelect[$pseudoFieldName] = [ + 'pseudoField' => $pseudoFieldName, + 'idCol' => $realFieldName, + 'field_name' => $field['name'], + 'bao' => $field['bao'], + 'pseudoconstant' => $field['pseudoconstant'], + ]; + } + + $this->_tables[$field['table_name']] = 1; + $this->_element[$realFieldName] = 1; + $this->_select[$field['name']] = str_replace('civicrm_contact.', 'contact_a.', "{$field['where']} as `$realFieldName`"); + } + + /** + * Is this pseudofield a foreign key constraint. + * + * We are trying to cautiously expand our pseudoconstant handling. This check allows us + * to extend to a narrowly defined type (and then only if the pseudofield is in the fields + * array which is done for contributions which are mostly handled as pseudoconstants. + * + * @param $fieldSpec + * + * @return bool + */ + protected function isPseudoFieldAnFK($fieldSpec) { + if (empty($fieldSpec['FKClassName']) + || CRM_Utils_Array::value('keyColumn', $fieldSpec['pseudoconstant']) !== 'id' + || CRM_Utils_Array::value('labelColumn', $fieldSpec['pseudoconstant']) !== 'name') { + return FALSE; + } + return TRUE; } } diff --git a/CRM/Contribute/BAO/Query.php b/CRM/Contribute/BAO/Query.php index 4519543a86..b6063aef9b 100644 --- a/CRM/Contribute/BAO/Query.php +++ b/CRM/Contribute/BAO/Query.php @@ -69,14 +69,6 @@ class CRM_Contribute_BAO_Query extends CRM_Core_BAO_Query { $query->_tables['civicrm_contribution'] = $query->_whereTables['civicrm_contribution'] = 1; } - // get financial_type - if (!empty($query->_returnProperties['financial_type'])) { - $query->_select['financial_type'] = "civicrm_financial_type.name as financial_type"; - $query->_element['financial_type'] = 1; - $query->_tables['civicrm_contribution'] = 1; - $query->_tables['civicrm_financial_type'] = 1; - } - // get accounting code if (!empty($query->_returnProperties['accounting_code'])) { $query->_select['accounting_code'] = "civicrm_financial_account.accounting_code as accounting_code"; diff --git a/tests/phpunit/CRM/Contribute/BAO/QueryTest.php b/tests/phpunit/CRM/Contribute/BAO/QueryTest.php index 928ac2bd17..b3e27b9bca 100644 --- a/tests/phpunit/CRM/Contribute/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/QueryTest.php @@ -14,10 +14,14 @@ class CRM_Contribute_BAO_QueryTest extends CiviUnitTestCase { * - financial_type. * * @param string $sort + * @param bool $isUseKeySort + * Does the order by use a key sort. A key sort uses the mysql 'field' function to + * order by a passed in list. It makes sense for option groups & small sets + * but may not do for long lists like states - performance testing not done on that yet. * * @dataProvider getSortFields */ - public function testSearchPseudoReturnProperties($sort) { + public function testSearchPseudoReturnProperties($sort, $isUseKeySort) { $contactID = $this->individualCreate(); $this->contributionCreate(['contact_id' => $contactID, 'financial_type_id' => 'Campaign Contribution']); $this->contributionCreate(['contact_id' => $contactID, 'financial_type_id' => 'Donation']); @@ -28,8 +32,12 @@ class CRM_Contribute_BAO_QueryTest extends CiviUnitTestCase { ]; $queryObj = new CRM_Contact_BAO_Query($params); + $sql = $queryObj->getSearchSQL(0, 0, $sort . ' asc'); + if ($isUseKeySort) { + $this->assertContains('field(', $sql); + } try { - $resultDAO = $queryObj->searchQuery(0, 0, $sort . ' asc'); + $resultDAO = CRM_Core_DAO::executeQuery($sql); $this->assertTrue($resultDAO->fetch()); $this->assertEquals(1, $resultDAO->N); } @@ -45,12 +53,13 @@ class CRM_Contribute_BAO_QueryTest extends CiviUnitTestCase { */ public function getSortFields() { return [ - ['payment_instrument'], - ['individual_prefix'], - ['communication_style'], - ['gender'], - ['state_province'], - ['country'], + ['financial_type', TRUE], + ['payment_instrument', TRUE], + ['individual_prefix', TRUE], + ['communication_style', TRUE], + ['gender', TRUE], + ['state_province', FALSE], + ['country', FALSE], ]; } diff --git a/tests/phpunit/CRM/Export/BAO/ExportTest.php b/tests/phpunit/CRM/Export/BAO/ExportTest.php index fe4ae5b9fc..c6ac775b34 100644 --- a/tests/phpunit/CRM/Export/BAO/ExportTest.php +++ b/tests/phpunit/CRM/Export/BAO/ExportTest.php @@ -2579,7 +2579,7 @@ class CRM_Export_BAO_ExportTest extends CiviUnitTestCase { 'world_region' => 'world_region varchar(128)', 'url' => 'url varchar(128)', 'phone_type_id' => 'phone_type_id varchar(16)', - 'financial_type' => 'financial_type varchar(64)', + 'financial_type' => 'financial_type varchar(255)', 'contribution_source' => 'contribution_source varchar(255)', 'receive_date' => 'receive_date varchar(32)', 'thankyou_date' => 'thankyou_date varchar(32)', -- 2.25.1