-
Notifications
You must be signed in to change notification settings - Fork 179
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
Different solution for ONLY_FULL_GROUP_BY (see #406) (Issue #80) #407
Conversation
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.
I reverted the switch to xenial as this was introduced by me and is not actually needed.
@SMillerDev any comments or can we merge this? |
@marco44 could you check this pr and do a review, it has been hanging around for quite some time now but I think it's still relevant |
Yeah, that's way much better than what previous queries did (functional dependency on PK in group by is risky and all SQL engines behave differently on this, so better not rely on it). It's not going to be that costlier (microseconds probably) anyway |
closed it by accident :/ |
…xtcloud#406) Signed-off-by: Talon <talontalon24@gmail.com>
Signed-off-by: Talon <talontalon24@gmail.com>
Changed - Basic Media-RSS support (#599) - Database index improvements (#637) Fixed - Call to a member function getUrlHash() on null" when adding a feed (#640) - Don't install symfony/console via composer (#636) - Fix for for ONLY_FULL_GROUP_BY (see #406) (Issue #80) (#407) - Catch invalid feeds (#646) Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
This is a different approach to #80 .
Here, we apply the aggregate functions in a subquery and get the defails from the
feeds
table in the outer query.This makes it easer to see what we are counting and by what we are grouping by. Also, we don't have to include every column from
feeds
in thegroup by
.This uses subqueries rather than a
with
clause as MySQL only supports that in version 8 upwards.Also, the table of origin has been added to
unread
, as all other columns are referenced this way.This PR is meant to be an alternative to #406 in consultation with @Grotax to put both ways up for discussion.