CRM-21362: Mailing summary report group by MySQL 5.7 error

[monishdeb]

Overview

Steps to replicate the error:

1. Go to Mailing Summary Report (template/instance)
2. Ensure that 'End Date' is not selected in Columns section.
3. Select 'End Date' in under Sorting form
4. Submit the Criteria

Before

Throw DB Error: unknown error
nativecode=1055 ** Expression #1 of ORDER BY clause is not in GROUP BY clause and contains nonaggregated column 'civicrm_db.mailing_job_civireport.end_date' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by|#1 of ORDER BY clause is not in GROUP BY clause and contains nonaggregated column 'civicrm_db.mailing_job_civireport.end_date' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by

After

Fixes the error, but note that chosen ORDER column is now appended in GROUP BY clause

Technical Details

You need MySQL 5.7 setup to test this. Also, this patch has some additional optimisation changes

Comments

This might be affecting other Reports where nonaggregated ORDER BY columns were not present in GROUP BY.

---

• CRM-21362: Mailing summary report group by MySQL 5.7 error

[seamuslee001]

@monishdeb you may want to check out this PR as well #10934 i'm fairly certain that its whats preventing some of these issues from poping up through the 1604 box

[monishdeb]

@seamuslee001 you I did and made some comments there. So as per both the patch we are introducing a two separate fn to set/unset ONLY_FULL_GROUP_BY in session as per mysql version respectively. Once #10934 gets merged I will add a UT for this fix

[lcdservices]

Looks good. Mailing summary report no longer throws group by error. Breaking this out into separate functions to determine group by mode and apply changes to report order by clauses is a good way to handle it.

[lcdservices]

@Monish just ran across an unintended consequence of this change that I didn't notice in my original testing. the start/end date field is derived from the job table, not the mailing table. when we add the end date to the group by clause, mailings with more than one job (large mailings that were multi-threaded) show up in the report as multiple rows.

so although I think the general utility function is helpful and my be used in other reports, in this particular case, we need to handle the start and end dates differently -- the start date should be set to MIN() and the end date to MAX() so that we get the earliest/latest job dates for the mailing -- and not group on them.

also -- another modified file was added to this PR, and I don't think it's relevant. it was likely intended for a different PR.

[lcdservices]

I think this should be MIN. We want the earliest start date, and latest end date.

[lcdservices]

@Monish latest changes look good. we're now pulling distinct mailing records and do not get group by errors. @colemanw can you review and merge?

[seamuslee001]

@monishdeb should the 2nd half of that 2nd test be && in_array not && !in_array as we want it if ONLY_FULL_GROUP_BY is in the array right?

[monishdeb]

@seamuslee001 no because the second part (!empty($sqlModes) && !in_array('ONLY_FULL_GROUP_BY', $sqlModes)) is for Mysql 5.7 where if ONLY_FULL_GROUP_BY is intentionally not present (i.e. !in_array) in sql modes then return TRUE which means GroupByMode is disabled.

[seamuslee001]

so you expect then in would just jump to the return in L93 because the array_search in 89 won't find it then right?

[monishdeb]

yes .. exactly. Also this logic is already written and tested earlier. See above in Contact/BAO/Query.php from where this condition was moved

[seamuslee001]

ok no worries makes sense then

[seamuslee001]

Agree with the PR, just want Monish to confirm that if statement but i think this is good to merge

[seamuslee001]

ping @eileenmcnaughton @colemanw this looks good to me

[seamuslee001]

ping @totten this seems to have been tested by Brian and fixes a problem