Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes #108: CActiveRecord::count() ignore 'together' relation option #1195

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants

@ghost ghost assigned cebe and samdark Sep 3, 2012

Owner

samdark commented Sep 8, 2012

Can't accept this one without unit-tests. It's very likely that it will result in wrong numbers because of using JOIN without DISTINCT.

Numbers can be wrong only when records multiply in a JOINs which is not the case for 1-to-1 relations.

DISTINCT slows down queries dramatically for huge tables. So it's not used when not needed: for BELONGS_TO and HAS_ONE relations.

For example, CGridview with pagination uses count() and findAll() with the same criteria. Since count() totally ignores "together" option these queries has different JOINs which is wrong.
So now the only way to have fast CGridview with huge tables - to avoid "with" and preload relations some other way.

Contributor

creocoder commented Sep 8, 2012

@MonkeyMaster The main thing @samdark say is unit tests needed. All other fine with this AF::count() optimization.

I think CActiveRecordTest.testRelationalCount() should fit

Owner

cebe commented Sep 9, 2012

@MonkeyMaster if it passed before and you fixed something and it still passes it definitively does not cover your issue ;)

Owner

samdark commented Sep 9, 2012

The following tests are needed:

  1. Count results for different combinations of models and relation types. Including usage of limit and setting together on and off.
  2. Expected SQL tests.

Could somebody help with these unit tests?..
This issue also affects CActiveDataProvider/CGridView performance so maybe someone else is interested

Contributor

creocoder commented Sep 10, 2012

I will make unit tests for it, but little later.

Owner

samdark commented Dec 5, 2015

Can't accept it w/o unit tests and since there are none for years, closing.

@samdark samdark closed this Dec 5, 2015

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