Skip to content
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

Fix Export when full group by mode is used #13124

Merged
merged 2 commits into from
Nov 20, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 19, 2018

Overview

Fixes an export performance issue in mysql Full Group By mode

Before

Running ExportIMTest horrendously slow with Full Group By enabled due to returning 7776 rows for php processing rather than 1

After

Test works quickly. Query is marked as not meeting the full group by standard using disable & re-enabled full group by.

Technical Details

After upgrading locally to mysql 5.7 I found that the exportIM test
was taking so long locally that I was killing it rather than finding
out how long it would take.

Digging into it I found that we were changing the query so that
attempts to group by contact ID were being nullified when full group by
is enabled. This meant that we were winding up with
7776 rows being retrieved to reflect a grid of permutations, which was
being iterated down to 1 in php.

In general the practice of altering the group by to meet this new
standard is one that we determined to be
causing problems and we discontinued / rolled back

Comments

Note test coverage is strong across the changed code

@seamuslee001

After upgrading locally to mysql 5.7 I found that the exportIM test
was taking so long locally that I was killing it rather than finding
out how long it would take.

Digging into it I found that we were changing the query so that
attempts to group by contact ID were being nullified when full group by
is enabled. This meant that we were winding up with
7776 rows being retrieved to reflect a grid of permutations, which was
being iterated down to 1 in php.

In general the practice of altering the groupby to meet this new
standard is one that we determined to be
causing problems and we discontinued / rolled back
@civibot
Copy link

civibot bot commented Nov 19, 2018

(Standard links)

@civibot civibot bot added the master label Nov 19, 2018
@seamuslee001
Copy link
Contributor

This looks good test coverage seems to be reasonable. The changes make sense and are in line with what we have been doing Merging

@seamuslee001 seamuslee001 merged commit d62cc08 into civicrm:master Nov 20, 2018
@seamuslee001 seamuslee001 deleted the export_fgb branch November 20, 2018 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants