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

Eager loading to prevent N+1 #36

Closed
TRPB opened this issue Oct 18, 2016 · 15 comments
Closed

Eager loading to prevent N+1 #36

TRPB opened this issue Oct 18, 2016 · 15 comments

Comments

@TRPB
Copy link
Member

TRPB commented Oct 18, 2016

When doing something like

foreach ($userMapper as $user) {
 echo $user->address->city;
}

It would be useful to eager load addresses and cache them. Currently this will issue one query for each of the users to find the address, what we really want to do is pass result from fetching the $users object and on the first call to $user->address run $addressMapper->find(['userId' => implode($users->id)]) (i realize that's not valid code) and update the cache in $userMapper with objects that contain the $address object.

Not a simple task but this would be the biggest single performance increase we'll get.

This behavior isn't always better but most of the time if you query a set of objects, you'll use them in a loop and do the same thing with each one, but it might be worth having a way to disable the behavior for individual mappers.

TRPB added a commit that referenced this issue Dec 22, 2017
@TRPB
Copy link
Member Author

TRPB commented Dec 22, 2017

I've implemented this but there's no test (as it involves digging into the inner workings and checking state. Will need to mock the datasource to do it).

@TRPB TRPB closed this as completed Dec 22, 2017
@solleer
Copy link
Collaborator

solleer commented Dec 24, 2017

This seems to have broken testManyManySaveIntermediateMultiple, testObjectGraphSaveDeep, and testManyManySave for the SQLite tests

@solleer solleer reopened this Dec 24, 2017
@TRPB
Copy link
Member Author

TRPB commented Dec 24, 2017

was this fixed by 693d98e ?

I can't replicate this, all tests are passing for me. Does it produce any errors?

@solleer
Copy link
Collaborator

solleer commented Dec 24, 2017

This is what the tests give me

There was 1 error:

  1. SqliteDatabaseTest::testManyManySaveIntermediateMultiple
    Undefined offset: 2
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\relation\one.php:59
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\relation\one.php:35
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\relation\one.php:76
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\relation\manymany.php:99
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\relation\manymany.php:34
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\lib\entity.php:26
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\lib\entity.php:18
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\maphper\maphper.php:98
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\tests\MySqlDatabaseTest.php:1019
    --
    There were 2 failures:
  2. SqliteDatabaseTest::testObjectGraphSaveDeep
    Failed asserting that 1 matches expected 2.
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\tests\MySqlDatabaseTest.php:235
  3. SqliteDatabaseTest::testManyManySave
    Failed asserting that 1 matches expected 2.
    C:\Users\Solle\OneDrive\Documents\Programing Stuff\Web development\serverRoot\GitHub\Maphper\tests\MySqlDatabaseTest.php:866

@TRPB
Copy link
Member Author

TRPB commented Dec 24, 2017

Really strange. I'm seeing that test pass. What PHP version are you using? I can only thing it can be the SQLite implementation.

I'm on

PHP 7.2.0 (cli) (built: Dec  5 2017 18:56:10) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2017 Zend Technologies
    with Xdebug v2.6.0alpha1, Copyright (c) 2002-2017, by Derick Rethans

@solleer
Copy link
Collaborator

solleer commented Dec 24, 2017

I'm on 7.0.10

@TRPB
Copy link
Member Author

TRPB commented Dec 24, 2017

The only version of 7.0 I could find an quickly install is 7.0.26, tests still pass. It might be a linux/windows difference unfortunately I don't have a windows machine I can easily test with

@TRPB
Copy link
Member Author

TRPB commented Dec 24, 2017

Nevermind, I had installed php7.0 but phpunit was still using the original executable. I've now replicated the error though I'm not sure what difference it should make

@TRPB
Copy link
Member Author

TRPB commented Dec 24, 2017

I don't think I know enough about SQLite to fix this. It seems to be a problem with the IN() query SELECT * FROM movie WHERE mid IN ( :mid0) which doesn't appear to yield any results. Is IN() not supported in older SQLite versions?

@solleer
Copy link
Collaborator

solleer commented Dec 24, 2017

I can't find anything about any changes to SQLite. @tontof Do you know what's wrong?

@tontof
Copy link
Contributor

tontof commented Dec 25, 2017

Is there a way to disable only MySQL tests ? I do not have a MySQL server running to test.
If not I will look to install one to run tests.

@TRPB
Copy link
Member Author

TRPB commented Dec 25, 2017

You can use phpunit --filter followed by the name of a class or individual test name

@tontof
Copy link
Contributor

tontof commented Dec 25, 2017

As SqliteDatabaseTest extends MySqlDatabaseTest it does not work using only --filter but I have also commented the mysql part in constructor of MySqlDatabaseTest and now I have the same results as @solleer with

HP 7.0.22-0ubuntu0.16.04.1 (cli) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.0.22-0ubuntu0.16.04.1, Copyright (c) 1999-2017, by Zend Technologies
    with Xdebug v2.4.0, Copyright (c) 2002-2016, by Derick Rethans

I won't have time this afternoon, but tomorrow I will have a look to try to understand the problem!

$ phpunit --filter SqliteDatabaseTest

PHPUnit 6.2.3 by Sebastian Bergmann and contributors.

.......F..................................F....E......            54 / 54 (100%)

Time: 49.13 seconds, Memory: 14.00MB

There was 1 error:

1) SqliteDatabaseTest::testManyManySaveIntermediateMultiple
Undefined offset: 2

Maphper/maphper/relation/one.php:59
Maphper/maphper/relation/one.php:35
Maphper/maphper/relation/one.php:76
Maphper/maphper/relation/manymany.php:99
Maphper/maphper/relation/manymany.php:34
Maphper/maphper/lib/entity.php:26
Maphper/maphper/lib/entity.php:18
Maphper/maphper/maphper.php:98
Maphper/tests/MySqlDatabaseTest.php:1019

--

There were 2 failures:

1) SqliteDatabaseTest::testObjectGraphSaveDeep
Failed asserting that 1 matches expected 2.

Maphper/tests/MySqlDatabaseTest.php:235

2) SqliteDatabaseTest::testManyManySave
Failed asserting that 1 matches expected 2.

Maphper/tests/MySqlDatabaseTest.php:866

ERRORS!
Tests: 54, Assertions: 176, Errors: 1, Failures: 2.

@tontof
Copy link
Contributor

tontof commented Dec 26, 2017

I've found the "problem", it's the same as #24 (comment)
If I change in the SqliteDatabaseTest file the PDO attribute error mode with PDO::ERRMODE_EXCEPTION
it works and all tests pass

The problem is that PDO error are shown and the tests result is:

PHPUnit 6.2.3 by Sebastian Bergmann and contributors.

........SQLSTATE[HY000]: General error: 1 table blog_126ac9 has no column named name..............................................            54 / 54 (100%)

Time: 45.64 seconds, Memory: 14.00MB

OK (54 tests, 183 assertions)

@tontof
Copy link
Contributor

tontof commented Dec 26, 2017

OK I think I've fixed the problem with this PR #53

@solleer solleer added this to the Version 1.0 milestone Dec 26, 2017
@TRPB TRPB closed this as completed in #53 Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants