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

[2.0.0] Refactored mvc/model/resultset to work with AppendIterator, prevent query re-execution and optimisations #10100

Merged
merged 1 commit into from
May 1, 2015

Conversation

patrick-zippenfenig
Copy link
Contributor

The current resultset implementation does not work with appendIterator (added unit-test).

"valid()" must not move the cursor to the next position. Now: "next()" and "seek()" move the cursor and prepare the row for hydration. Hydration is handled in "current()" instead of "valid()".

It is now safe to call valid() and current() multiple times. current() keeps the activeRow in memory. See #2502

Array access does not trigger re-execution if the same index is called multiple times or the index is ahead of current pointer:

$robots = Robots::find();
for($i = 0; $i < count($robots); ++$i) {
  echo $robots[$i]->name;
}

See #10057

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
…uery re-execution and optimisations
@KorsaR-ZN
Copy link
Contributor

👍

@patrick-zippenfenig
Copy link
Contributor Author

I've been using it in active development for few days now and did not encounter any errors. Unit-test coverage should be sufficient. Especially newly added tests make sure re-executions only occur if necessary.

Let me know if I can provide additional tests

@Green-Cat
Copy link
Contributor

👍

1 similar comment
@sergeyklay
Copy link
Contributor

👍

andresgutierrez added a commit that referenced this pull request May 1, 2015
[2.0.0] Refactored mvc/model/resultset to work with AppendIterator, prevent query re-execution and optimisations
@andresgutierrez andresgutierrez merged commit fe2010e into phalcon:2.0.x May 1, 2015
@andresgutierrez
Copy link
Contributor

Thanks

@KorsaR-ZN
Copy link
Contributor

@patrick-zippenfenig
It's not working :(

$data = Robots::find();

var_dump($data->getFirst()); // bool(false)
var_dump($data->getLast()); // bool(false)

@patrick-zippenfenig
Copy link
Contributor Author

@KorsaR-ZN I would be surprised if getFirst and getLast failed on simple tests. Should be covered by unit tests. Are array access operations and iterators working? I'll check later today

@KorsaR-ZN
Copy link
Contributor

@patrick-zippenfenig
I think, error in the method Phalcon\Mvc\Model\Resultset\seek()

Test 1

$data = Robots::find(['limit' => 3]);

var_dump(count($data)) // int(3)
var_dump($data->getLast()); // object(Robots)...
var_dump($data->getFirst()); // object(Robots)...

foreach ($data as $row) {
    echo $row->id;
} 
// echo 1,2,3

Test 2

$data = Robots::find(['limit' => 32]);

var_dump(count($data)) // int(32)
var_dump($data->getLast()); // object(Robots)...
var_dump($data->getFirst()); // object(Robots)...

foreach ($data as $row) {
    echo $row->id;
} 
// echo 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,

Test 3

$data = Robots::find();

var_dump(count($data)) // int(53)
var_dump($data->getLast()); // bool(false)
var_dump($data->getFirst()); // object(Robots)...

foreach ($data as $row) {
    echo $row->id;
} 
// echo 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,

Test 4

$data = Robots::find();

var_dump(count($data)) // int(53)

foreach ($data as $row) {
    echo $row->id;
} 
// echo 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,

var_dump($data->getLast()); // object(Robots)...
var_dump($data->getFirst()); // object(Robots)...

Test 5

$data = Robots::find();

var_dump(count($data)) // int(53)

echo gettype($data[19]) . PHP_EOL; // object [OK]
echo gettype($data[33]) . PHP_EOL; // object [OK]
echo gettype($data[45]) . PHP_EOL; // boolean [FAIL]
echo gettype($data[45]) . PHP_EOL; // boolean [FAIL]
echo gettype($data[51]) . PHP_EOL; // boolean [FAIL]

echo PHP_EOL;

echo gettype($data[0]) . PHP_EOL;   // object [OK]
echo gettype($data[5]) . PHP_EOL;   // object [OK]
echo gettype($data[19]) . PHP_EOL;  // object [OK]
echo gettype($data[20]) . PHP_EOL;  // object [OK]
echo gettype($data[29]) . PHP_EOL;  // object [OK]
echo gettype($data[31]) . PHP_EOL;  // object [OK]
echo gettype($data[45]) . PHP_EOL;  // object [OK]

echo PHP_EOL;

for ($i = 0, $n = count($data); $i < $n; $i++) {
    echo '#' . $i . ' ' . gettype($data[$i]) . PHP_EOL;
}

// #0 object
// #1 object
// #2 object
// #3 object
// #4 object
// #5 object
// #6 object
// #7 object
// #8 object
// #9 object
// #10 object
// #11 object
// #12 object
// #13 object
// #14 object
// #15 object
// #16 object
// #17 object
// #18 object
// #19 object
// #20 object
// #21 object
// #22 object
// #23 object
// #24 object
// #25 object
// #26 object
// #27 object
// #28 object
// #29 object
// #30 object
// #31 object
// #32 object
// #33 object
// #34 object
// #35 object
// #36 object
// #37 object
// #38 object
// #39 object
// #40 object
// #41 object
// #42 object
// #43 object
// #44 object
// #45 object
// #46 object
// #47 object
// #48 object
// #49 object
// #50 object
// #51 object
// #52 object

@patrick-zippenfenig
Copy link
Contributor Author

@KorsaR-ZN I just submitted #10253. Should fix your mentioned problems. Sorry for the inconveniences.

@tmihalik
Copy link
Contributor

I think there's some bug here when using SQL JOINs. Monitoring database statements (PostgreSQL) log I see that query gets re-executed every time when trying to work with resultset. Was OK in 2.0.0.

$di = new Phalcon\DI\FactoryDefault;

$di->setShared('db', function () {
    return new Phalcon\Db\Adapter\Pdo\Postgresql([
        'host'     => 'localhost',
        'username' => 'user',
        'password' => 'pass',
        'dbname'   => 'dbname',
    ]);
});

class Users extends Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->setSchema('test');
        $this->setSource('users');
    }
}

class Comments extends Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->setSchema('test');
        $this->setSource('comments');
    }
}

$sql = 'SELECT Comments.*, Users.*
        FROM Comments
        INNER JOIN Users AS Users ON Users.id = Comments.user_id';

// first exec
$rows = $di->get('modelsManager')->createQuery($sql)->execute();

// this will re-execute the query
foreach ($rows as $row) {
}

// this will re-execute the query
$rows->toArray();

@phalcon @andresgutierrez

@andresgutierrez
Copy link
Contributor

@tmihalik It has to re-execute the queries because PDO does not support scrollable cursors consistently

This is mentioned in the docs: http://docs.phalconphp.com/en/latest/reference/models.html#model-resultsets

@tmihalik
Copy link
Contributor

@andresgutierrez I can't understand.

It was OK in 1.3.x, 2.0.0.
Without the JOIN it's also OK in 2.0.1.
Phalcon's philosophy is to be super fast. Running queries twice is not fast :(

@andresgutierrez
Copy link
Contributor

@patrick-zippenfenig Can you take a look?

@patrick-zippenfenig
Copy link
Contributor Author

@tmihalik How many times is the query executed in total? How many rows? It must be executed twice since PDO fetch can't rewind.

Documentation is also a bit wrong:

Storing large query results in memory could consume many resources, because of this, resultsets are obtained from the database in chunks of 32 rows reducing the need for re-execute the request in several cases also saving memory.

There is no chunking as far as I know, but queries with <=32 rows are fetched all at once.

@patrick-zippenfenig
Copy link
Contributor Author

@tmihalik "Joins" use resultset\complex, regular queries resultset\simple. Complex result-sets do not use 32 rows prefetch. I'll check if this can be implemented. Also the referred chunks of 32 rows might be a good feature to implement.

@tmihalik
Copy link
Contributor

@patrick-zippenfenig
#10100 (comment)

$rows = $di->get('modelsManager')->createQuery($sql)->execute(); // first exec
foreach ($rows as $row) { // second exec
}
$rows->toArray(); // third exec
$rows->toArray(); // fourth exec
$rows->toArray(); // fifth exec
foreach ($rows as $row) { // sixth exec
}
// ....
// total selected rows < 10

@patrick-zippenfenig
Copy link
Contributor Author

@tmihalik
Copy link
Contributor

@patrick-zippenfenig
Thank you!
I tried it, but unfortunately the same result.

@patrick-zippenfenig
Copy link
Contributor Author

Just to make sure: Did you run zephir compile/build? This commit should only prevent the second re-execution. I'll add the mentioned 32 row prefetch later

@tmihalik
Copy link
Contributor

I have runned the following commands:

$ git clone --branch=resultsetOptimisations https://github.com/patrick-zippenfenig/cphalcon.git
$ cd cphalcon
$ zephir build

@patrick-zippenfenig
Copy link
Contributor Author

I just enabled 32 row prefetch for complex-resultsets. Could you try again?

@tmihalik
Copy link
Contributor

@patrick-zippenfenig
Now it's OK, thank you!
Please send a pull request to 2.0.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

Successfully merging this pull request may close these issues.

None yet

6 participants