-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Speed up contribution results by removing join on civicrm_financial_type table when rendering search results. #13720
Conversation
(Standard links)
|
b5aa0a6
to
c918b39
Compare
test this please |
2 similar comments
test this please |
test this please |
@monishdeb @jitendrapurohit @seamuslee001 @colemanw really looking for someone who has time to help get this merged - I'd really like it to be in the 5.12 rc - it's less scary than it might seem because the test coverage is good on this - incl the pre-merged tests |
Tested the search forms with different combination of filters. Seems to work fine on all of them. Query formation before the PR -
After -
The output rows are same and unaffected. No errors noticed on any forms after applying the PR. Looks good to be merged 👍 |
@@ -77,7 +77,7 @@ | |||
</foreignKey> | |||
<field> | |||
<name>financial_type_id</name> | |||
<title>Financial Type</title> | |||
<title>Financial Type ID</title> |
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.
Why this change?
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.
We have done this with a few fields - it differentiates them in search builder. I'm OK with leaving out if you want
…ype to pseudoconstants Getting away from the hard-hack allows us to have more consistent metadata for generic fields
This is a very minor extension of an existing hack but we should de-hack at some point. Before this the qill is broken on payment instrument & contribution page too
… 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
c918b39
to
124f3c1
Compare
Overview
Speed up contribution results by removing join on civicrm_financial_type table when rendering search results.
Before
Join on civicrm_financial_type used to get financial type name = slower
After
Pseudoconstant used - faster
Technical Details
This can be done much more efficiently through the pseudoconstant mechanism.
Although it is an indexed join it is chosen at the expense of another index - so
your search on payment_instrument_id may not use an index on the filter field
due to this join.
Comments
Note that there are multiple unit tests covering filtering by & returning of financial type. Last time I worked in this part of the code I added the function api_v3_ContactTest::testPseudoFields to cover the pseudofields handled at that time. We hit some regressions around order by in search builder so this time I added
CRM_Contribute_BAO_QueryTest::testSearchPseudoReturnProperties
in preparation for this change