[DDC-1398] Extra-lazy get for indexed associations #706

Merged
merged 7 commits into from Jun 20, 2013

Conversation

Projects
None yet
4 participants
Contributor

sandermarechal commented Jun 20, 2013

If an association is EXTRA_LAZY and has an indexBy, then
you can call get() without loading the entire collection.

@sandermarechal sandermarechal [DDC-1398] Extra-lazy get for indexed associations
If an association is EXTRA_LAZY and has an indexBy, then
you can call get() without loading the entire collection.
523697d
Owner

beberlei commented Jun 20, 2013

<3

@stof stof commented on an outdated diff Jun 20, 2013

lib/Doctrine/ORM/Persisters/ManyToManyPersister.php
@@ -38,6 +38,24 @@ class ManyToManyPersister extends AbstractCollectionPersister
*
* @override
*/
+ public function get(PersistentCollection $coll, $index)
+ {
+ $mapping = $coll->getMapping();
+ $uow = $this->em->getUnitOfWork();
+ $persister = $uow->getEntityPersister($mapping['targetEntity']);
+
+ if (!isset($mapping['indexBy'])) {
+ throw new \BadMethodCallException("Selecting a collection by index is only supported on indexed collections.");
+ }
+
+ return current($persister->load(array($mapping['indexBy'] => $index), null, null, array(), 0, 1));
@stof

stof Jun 20, 2013

Member

you need to handle the case of non-existent keys properly: the Collection interface wants to get null in such case, not false (which is what current returns when the array is empty)

@Ocramius Ocramius commented on an outdated diff Jun 20, 2013

lib/Doctrine/ORM/PersistentCollection.php
@@ -517,6 +517,14 @@ public function indexOf($element)
*/
public function get($key)
{
+ if ( ! $this->initialized
+ && $this->association['fetch'] === Mapping\ClassMetadataInfo::FETCH_EXTRA_LAZY
+ && isset($this->association['indexBy'])
+ ) {
+ $persister = $this->em->getUnitOfWork()->getCollectionPersister($this->association);
@Ocramius

Ocramius Jun 20, 2013

Owner

I think you can get rid of $persister too here. Otherwise the PR looks clean!

Member

stof commented Jun 20, 2013

you could also implement containsKey this way, checking if an element is found

@beberlei beberlei and 1 other commented on an outdated diff Jun 20, 2013

...rine/Tests/ORM/Functional/ExtraLazyCollectionTest.php
@@ -512,6 +517,38 @@ public function testSliceOnDirtyCollection()
$this->assertEquals($qc + 1, $this->getCurrentQueryCount());
}
+ /**
+ * @group DDC-1398
+ */
+ public function testGetIndexByOneToMany()
+ {
+ $user = $this->_em->find('Doctrine\Tests\Models\CMS\CmsUser', $this->userId);
+ /* @var $user CmsUser */
+
+ $queryCount = $this->getCurrentQueryCount();
+
+ $user->articles->get($this->articleId);
@beberlei

beberlei Jun 20, 2013

Owner

can you test for the actual data being returned as well? in the second test as well

@stof

stof Jun 20, 2013

Member

and please also add a test for non-existent keys

sandermarechal added some commits Jun 20, 2013

@sandermarechal sandermarechal Return NULL for non-existent keys
The load() function already returns just one entity or NULL, so
the current() is not needed and the result can be returned directly.
3555007
@sandermarechal sandermarechal Test actual data returned by get() 647c5e2
@sandermarechal sandermarechal Get rid of variable 53c9ffd
Contributor

sandermarechal commented Jun 20, 2013

I think I fixed all the things you pointed out. I'm looking at implementing containsKey as well.

Contributor

sandermarechal commented Jun 20, 2013

I am not sure what the best way is to handle containsKey. I see three possible options:

  1. Simply call get from containsKey and check for NULL. This is simple to implement but it will load the actuall entity (which may have FETCH_EAGER associations, causing more data to be hydrated, etcetera).

  2. Query the database directly. This is what I have been working on so far. It works, but looks ugly. There is significant code overlap between containsKey and count and I don't see a non-ugly way to reduce it, except for option 3.

  3. Extend the count API to accept additional conditions and/or Criteria. In this case containsKey could simply use count with an extra condition and check if the result >= 0. Downside is that it is more work and that it is an addition to the API. On the upside, this sounds like a nice feature to have. People would be able to do things like this on a collection, and it would work properly on extra lazy collections as well:

    $numUsers = $group->users->count(array('active' => true));
    

So, what do you suggest?

Owner

beberlei commented Jun 20, 2013

@sandermarechal Lets keep containsKey for another time, we have something very useful here already. One thing I would like to see is the Persister Collections to use find() instead of findBy() to use the identity map, if the index-by is the identifier field. Otherwise its already awesome

Contributor

sandermarechal commented Jun 20, 2013

@beberlei: I'm sorry, I don't quite understand what you mean. I'm not using find() in this PR but I am using load() directly.

Do you mean fixing the TODO at BasicEntityPersister->load() on line 742?

Owner

beberlei commented Jun 20, 2013

@sandermarechal yes, please use the EntityManager#find instead if index-by == identifier field. This will greatly enhance performance in some use-cases.

Member

stof commented Jun 20, 2013

@sandermarechal regarding your propsal above, I don't think count() should be changed. It is the implementation of Countable

Contributor

sandermarechal commented Jun 20, 2013

@beberlei Done

@stof Good point. I didn't think about Countable. Anyway, I will make a separate PR for containsKey so this can be merged.

Owner

beberlei commented Jun 20, 2013

@sandermarechal i am afraid you need to add more tests now ;) the article stuff uses articleIds, so its never triggering the collection persister get calls anymore

Contributor

sandermarechal commented Jun 20, 2013

@beberlei Yup. Done.

@beberlei beberlei added a commit that referenced this pull request Jun 20, 2013

@beberlei beberlei Merge pull request #706 from sandermarechal/extra-lazy-get
[DDC-1398] Extra-lazy get for indexed associations
eaf8fd3

@beberlei beberlei merged commit eaf8fd3 into doctrine:master Jun 20, 2013

1 check failed

default The Travis CI build failed
Details

sandermarechal deleted the sandermarechal:extra-lazy-get branch Jun 20, 2013

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