Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

My old patches cherry-picked from master branch #351

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

dlnsk
Copy link

@dlnsk dlnsk commented Nov 2, 2015

Can't pass function name through setCallbacks because it encoding as option.

@timgws
Copy link
Collaborator

timgws commented Nov 2, 2015

Thanks for this PR, @dlnsk...

@dlnsk
Copy link
Author

dlnsk commented Nov 3, 2015

Next commit (1853d41) fix wrong count when groupBy used in query.

@Javier-Rotelli
Copy link

hey @dlnsk your first commit it's doing the same thing that i did here: #348 ?

@timgws
Copy link
Collaborator

timgws commented Nov 4, 2015

@dlnsk did you try setting the noGroupByOnCount option on QueryEngine? This should remove the group by before doing the count, which should lead to having the correct table count.

Doing:

        return \DB::table(\DB::raw('('.$originalBuilder->toSql().') as temp_tbl'))
                    ->mergeBindings($originalBuilder->getQuery())
                    ->count();

is not good, because it means a full table scan will need to be done for each search, sort and count, which will be extremely slow (http://dev.mysql.com/doc/refman/5.7/en/subquery-optimization.html)

@dlnsk
Copy link
Author

dlnsk commented Nov 4, 2015

@timgws it isn't the same. When I use groupBy I want know how many groups there are. Not how many items in all groups which noGroupByOnCount gives.
I can use setSearchWithAlias() but in my opinion it should be slower than subquery because need to send hundreds or even thousands records through network layer for php's count(). And it gives wrong result for language.infoFiltered ("filtered from MAX total entries") message.

@dlnsk
Copy link
Author

dlnsk commented Nov 4, 2015

@Javier-Rotelli yes ;o) but in little less lines

@Javier-Rotelli
Copy link

true :p
in this commit e035066 is a test for that functionality. maybe it helps

@dlnsk
Copy link
Author

dlnsk commented Nov 6, 2015

@Javier-Rotelli thanks but it already in main develop branch

@dlnsk dlnsk changed the title Problem with setCallbacks My old patches cherry-picked from master branch Mar 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants