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

mysql 5.7 sql_mode=only_full_group_by #10723

Closed
saeideng opened this Issue Jun 5, 2017 · 16 comments

Comments

Projects
None yet
7 participants
@saeideng
Member

saeideng commented Jun 5, 2017

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Version: cakephp 3.x

  • Platform and Target: mysql 5.7

What you did

EXPLAIN WHAT YOU DID, PREFERABLY WITH CODE EXAMPLES, HERE.

What happened

//Table1 hasmany Table2
//Table2 belongsto Table1
         $list=$this->Table1->find()->matching('Table2',
            function ($q) use ($some_id){
             return $q
                   ->where(['Table2.some_id' => $some_id]);
           }
         )
         ->distinct(['Table1.id'])

throw
Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column '.....' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by
just in mysql 5.7
but when use
->distinct(['Table1.id','Table2.id']) that resoled but result have unexpected row
that means we need first of matched row

distinct() make group by clause in this case
have a plan for fix it? or at least error/exception or something else ?

@markstory

This comment has been minimized.

Member

markstory commented Jun 5, 2017

Distinct is functionally the same as group by, which is why the queryBuilder emits that SQL. Unfortunately you're hitting a SQL mistake that Mysql has only recently started treating as an error. Given that this is a driver error I don't think we should attempt to mask or change the error message.

@markstory markstory added this to the 3.5.0 milestone Jun 5, 2017

@markstory markstory added the ORM label Jun 5, 2017

@saeideng

This comment has been minimized.

Member

saeideng commented Jun 6, 2017

Distinct was equal to group by before mysql 5.7 and should change now
Exist programs throw mysql error after updating mysql
I know we can disable this sql_mode

@markstory

This comment has been minimized.

Member

markstory commented Jun 6, 2017

I know we can disable this sql_mode

Yes, you'll have to do that in your MySQL server config, or update your queries. For more background information we have to rewrite DISTINCT with an array of fields into a GROUP BY because MySQL does not support the DISTINCT ON.

@danielzzz

This comment has been minimized.

danielzzz commented Jun 7, 2017

is there any workaround for that until there is a proper fix?

@markstory

This comment has been minimized.

Member

markstory commented Jun 7, 2017

No. The SQL you're asking the ORM to generate is invalid with the stricter SQL mode of MySQL. You'll need to either turn off the full group by mode, or get the query builder to generate a query that MySQL will run.

@dereuromark

This comment has been minimized.

Member

dereuromark commented Jul 19, 2017

I just encountered the same thing using the official ORM distinct() method in MySQL:

$Releases->find()->limit(10)->distinct('pr')
    ->where(['pr IS NOT' => null])
    ->find('list', ['keyField' => 'pr', 'valueField' => 'title'])
    ->toArray()

Causes

Expression #2 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'releaseapp.Releases.title' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by

I think we really should fix this. This worked fine locally due to the different version and suddenly blew up on production. I would expect the ORM to format this query as it would both on postgres and mysql (both versions) to get a valid SQL from it.

@inoas

This comment has been minimized.

Contributor

inoas commented Jul 19, 2017

I don't think we should fix on the framework level which has been practice in MySQL for ages up to 5.7. It is a really bad legacy behavior that is not supported in SqlServer, PostgreSQL, MariaDB and MySQL 5.7+ AFAIR. So it only affects "old" installations that did it wrong.

However, I think a warning to the docs about distinct and group by should be sufficient.

As for your case, I am not entirely sure but my gut feeling says: create a subquery with the distinct clause and then WHERE id IN onto that subquery to get the key/value list items.

@dereuromark

This comment has been minimized.

Member

dereuromark commented Jul 19, 2017

->distinct(['pr', 'title']) in my case is also sufficient enough, but my point is that this is rather hidden.
I wonder if we could auto-add the missing fields here rather than blowing up, if this is a bad practice to keep the old behavior. Maybe with some feature flag to enable?

//EDIT: due to the order statement, also created was missing and threw still an exception
Maybe ->distinct(['pr', 'title', 'created'])
But then I can also start dopping the distinct() clause anyway..^^ I wasnt too much interested in what of those possible duplicates, just any would have sufficed.

@saeideng

This comment has been minimized.

Member

saeideng commented Jul 19, 2017

I don't think we should fix on the framework level

but i think this must fix in framework level or at least a run time error on mysql 5.7 ...

@inoas

This comment has been minimized.

Contributor

inoas commented Jul 19, 2017

@saeideng a runtime warning (only with debug true?), if sql_mode is unset or not set to only_full_group_by) may help 👍 .

However, some legacy apps just running on with the flag not set/off, and they should continue to run.

@dereuromark

This comment has been minimized.

Member

dereuromark commented Jul 19, 2017

@ionas In that case couldnt we have that flag set on the Datasources config in app.php to trigger/enable it? As clear opt-in?

@saeideng

This comment has been minimized.

Member

saeideng commented Jul 20, 2017

i think we should have a tab/panel on debug_kit for report/alert for
1)warning
2)alert
3)exception
4)using deprecated method

so we can alert to developer about this within this panel if user not set bellow

Datasources config in app.php to trigger/enable it

@ravage84

This comment has been minimized.

Member

ravage84 commented Jul 21, 2017

For me, this is a (default) configuration issue with MySQL. Or better said a long overdue correction of behavior in MySQL.
I don't feel that a framework should (re-) set the MySQL configuration to enable this mis-behavior.

Side note: We ran into this, too. I temporarily set the necessary mode in MySQL, until we have fixed all places in our code.

I agree though, we could add a warning/note into related sections of the cook book.

@saeideng

This comment has been minimized.

Member

saeideng commented Jul 22, 2017

related : #8479

@saeideng

This comment has been minimized.

Member

saeideng commented Aug 8, 2017

if you have

//Table1 hasmany Table2
//Table2 belongsto Table1

I think when you are using group_by in Table1 side this error occur
->distinct(['Table1.id'])
but ->distinct(['Table2.id']) should return true result

@markstory markstory modified the milestones: 3.5.0, 3.5.1, 3.6.0 Aug 19, 2017

@dereuromark

This comment has been minimized.

Member

dereuromark commented Jan 30, 2018

Should we then close this in favor of #8479 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment