Skip to content
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

Having in query builder breaks total pages and total items of paginator #12111

Closed
Dublerq opened this issue Aug 8, 2016 · 19 comments
Closed

Comments

@Dublerq
Copy link

Dublerq commented Aug 8, 2016

Using having of a query builder forces it's paginator to show total pages and total items as if there was no having set.
Issue confirmed in 2.1.x and 3.0.0 on windows 7, php 5.6

How to reproduce:
/bootstrap.php

use Phalcon\Loader;
use Phalcon\Mvc\View;
use Phalcon\Mvc\Application;
use Phalcon\Di\FactoryDefault;
use Phalcon\Db\Adapter\Pdo\Mysql as DbAdapter;

// Register an autoloader
$loader = new Loader();
$loader->registerDirs(array(
    'models/',
))->register();

$di = new FactoryDefault();

$di->set('view', function () {
    $view = new View();

    $view->setViewsDir('../app/views/');

    return $view;
});

$di->set('url', function () {
    $url = new UrlProvider();

    return $url;
});

$di->set('db', function () {
        return new DbAdapter(array(
            "host"     => "localhost",
            "username" => "root",
            "password" => "",
            "dbname"   => "phalcon_test"
        ));
    });

// Setup the view component

$application = new Application($di);

// Handle the request
//$response = $application->handle();

$query = new \Phalcon\Mvc\Model\Query\Builder();
$query->setDI($di);
$query->columns('*, COUNT(*) as stock_count');
$query->from('A');
$query->groupBy('name');
$query->having('SUM(A.stock) > 0');
var_dump($query->getQuery()->getSql());
$paginator = new \Phalcon\Paginator\Adapter\QueryBuilder(array(
    "builder" => $query,
    "limit" => 1,
    "page" => 2
));
var_dump($paginator->getPaginate());
//$response->send();

/model/a.php:

class A extends Phalcon\Mvc\Model
{    
}

sql database dump:

SET SQL_MODE = "NO_AUTO_VALUE_ON_ZERO";
SET time_zone = "+00:00";

CREATE TABLE `a` (
  `id` int(11) NOT NULL,
  `name` varchar(32) NOT NULL,
  `stock` int(11) NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

INSERT INTO `a` (`id`, `name`, `stock`) VALUES
(1, 'Apple', 2),
(2, 'Carrot', 6),
(3, 'pear', 0);


ALTER TABLE `a`
  ADD PRIMARY KEY (`id`);


ALTER TABLE `a`
  MODIFY `id` int(11) NOT NULL AUTO_INCREMENT, AUTO_INCREMENT=4;

Expected result:
image

Query builder result:

image

@Dublerq Dublerq changed the title Having in query builder brakes total pages and total items of paginator Having in query builder breaks total pages and total items of paginator Aug 8, 2016
@Dublerq
Copy link
Author

Dublerq commented Aug 10, 2016

@sergeyklay @Jurigag Can you check it, please? Maybe I am doing something wrong...

@Jurigag
Copy link
Contributor

Jurigag commented Aug 10, 2016

Count is done BEFORE having clause. And it's not phalcon issue, it's just in all SQL based databases. It's more like an NFR/Enhancment than a bug imho.

@Dublerq
Copy link
Author

Dublerq commented Aug 10, 2016

And IMHO it's a bug because SQL response is different than paginator expected result. You can use neither aliases nor SUM/COUNT... functions in WHERE so HAVING is the only way here and it must be working properly

@Jurigag
Copy link
Contributor

Jurigag commented Aug 10, 2016

What ? But you didnt do COUNT bro. You just did select. Check COUNT(*) on this query. It will return 3 :).

It's not a bug as it is, HAVING is done AFTER COUNT in SQL.

But sure something has to be done to make it working properly and as it should be.

@Dublerq
Copy link
Author

Dublerq commented Aug 11, 2016

Oh, right, you are right. I didn't get whant you mean. Neither did I know about that count problem in SQLs, sorry. But still, paginator is working differently than expected so it should be somehow improved. Do you have an idea how can we do it?

@sergeyklay
Copy link
Contributor

@Dublerq

it should be somehow improved

What do you suggest

@Dublerq
Copy link
Author

Dublerq commented Aug 12, 2016

Let's count really existing items of query result in php instead of SQL. It should help (It's a guess, I don't know)
There was another issue but with same workaround:
#2065 (comment)

@Izopi4a
Copy link
Member

Izopi4a commented Aug 13, 2016

i know its my PR, but still could be useful.

#12041

@Dublerq change the file ( cphalcon/phalcon/paginator/adapter/querybuilder.zep ) like like its in the PR, before you compile phalcon, until 3.1 release ( when its gonna be merged ) and do it like in the tests provided / or in description.

@Jurigag
Copy link
Contributor

Jurigag commented Aug 13, 2016

But this WONT fix this @Izopi4a The only way is to do:

SELECT COUNT(*) FROM (
    -- OUR SELECT STATEMENT WHICH WILL RETURN THIS ROWS
)

But phalcon dont support queries like this.

Perhaps maybe - if we have having, then get builded sql query, and do raw query as i wrote above ?

@BesedinSasha
Copy link
Contributor

Have the same problem. When using HAVING in QueryBuilder, pagination execute wrong query. That's why we have to use subquery for true pagination results as @Jurigag wrote above.

@Jurigag
Copy link
Contributor

Jurigag commented Sep 30, 2016

As i wrote. Pagination executes PROPER QUERY. Just having works AFTER selecting, count etc. This is not a bug as it is. This is just how sql works. The only solution to this is what i wrote above.

@Jurigag
Copy link
Contributor

Jurigag commented Mar 14, 2017

Well, i am trying to fix this, but there is a little problem, it would be to heavy operation to figure out which columns to select imho, like get primary keys, figure out alias, user columnMap etc etc, much simpler and faster will be just adding property columns where user will provide for example "Robots.id" and this column will be used for:

SELECT COUNT(*) FROM {
    SELECT THIS COLUMNS FROM MODEL {

    } as T1
}

Don't see really better idea, adding like auomatic way to making which columns we only need for selecting count would be way too heavy operation(get model instances, get first model instance, figure out alias, figure out primary keys, check against column map).

We can't use really columns from original query builder cuz it will select pretty much everything from database.

Anyway i highly doubt that it will have good performance anyway.

@Besedin86 @Izopi4a @Dublerq

@Izopi4a
Copy link
Member

Izopi4a commented Mar 14, 2017

i still beleave my idea should be fine, simply because it will be up to the developer to handle his stuff

@Jurigag
Copy link
Contributor

Jurigag commented Mar 14, 2017

Hmmm, maybe it can be fine too, i don't know really, it's not hard to do it like my idea, but still developer would need provide columns for count select.

@Izopi4a
Copy link
Member

Izopi4a commented Mar 14, 2017

i absolutely agree with you, i am just afraid that implementing such a dynamic options for pagination might become too complicated / complex to people to use them.

It just orm should be "1 liner" in my mind, thats all

But i dont mind, if someone come up with a plan we can agree who to cook it up

@Jurigag
Copy link
Contributor

Jurigag commented Mar 14, 2017

I mean something like:

'builder'    => $builder, // some builder with Robots aliast and RobotsParts alias
        'limit'      => 20,
        'page'       => $currentPage,
        'columns' => 'Robots.id'

And then for getting count there will be query:

SELECT COUNT(*) FROM {
    SELECT Robots.id FROM // our columns option
    // the rest of query from builder
} as T1

Any dynamic way to figure out this columns for this nested select will cause slowdown really.

@Jurigag
Copy link
Contributor

Jurigag commented Mar 15, 2017

This thing should be consider as fix or new feature?

@Jurigag Jurigag mentioned this issue Mar 15, 2017
3 tasks
@Jurigag
Copy link
Contributor

Jurigag commented Mar 17, 2017

#12712

Here is my PR for this thing, let me know what you think.

@Jurigag
Copy link
Contributor

Jurigag commented Jun 23, 2017

This can be closed. It's implemented in 3.2.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants