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

Comments

Projects
None yet
5 participants
@Dublerq

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 from Having in query builder brakes total pages and total items of paginator to Having in query builder breaks total pages and total items of paginator Aug 8, 2016

@Dublerq

This comment has been minimized.

Show comment
Hide comment
@Dublerq

Dublerq Aug 10, 2016

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

Dublerq commented Aug 10, 2016

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Aug 10, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Dublerq

Dublerq 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

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Aug 10, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Dublerq

Dublerq 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?

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

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Aug 12, 2016

Member

@Dublerq

it should be somehow improved

What do you suggest

Member

sergeyklay commented Aug 12, 2016

@Dublerq

it should be somehow improved

What do you suggest

@Dublerq

This comment has been minimized.

Show comment
Hide comment
@Dublerq

Dublerq 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)

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

This comment has been minimized.

Show comment
Hide comment
@Izopi4a

Izopi4a Aug 13, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Aug 13, 2016

Member

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 ?

Member

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 ?

@Besedin86

This comment has been minimized.

Show comment
Hide comment
@Besedin86

Besedin86 Sep 30, 2016

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.

Besedin86 commented Sep 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Sep 30, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 14, 2017

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@Izopi4a

Izopi4a Mar 14, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 14, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Izopi4a

Izopi4a Mar 14, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 14, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 15, 2017

Member

This thing should be consider as fix or new feature?

Member

Jurigag commented Mar 15, 2017

This thing should be consider as fix or new feature?

@Jurigag Jurigag referenced this issue Mar 15, 2017

Merged

3.2.x paginator having #12712

3 of 3 tasks complete
@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 17, 2017

Member

#12712

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

Member

Jurigag commented Mar 17, 2017

#12712

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 23, 2017

Member

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

Member

Jurigag commented Jun 23, 2017

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

@sergeyklay sergeyklay closed this Jun 23, 2017

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