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

Lazy criteria for ManyToMany collection #1033

Open
wants to merge 1 commit into
base: old-prototype-3.x
Choose a base branch
from

Conversation

bakura10
Copy link
Member

This continues my previous work on making Criteria most efficient.

Currently we are wrapping matching calls on repositories and matching calls on EXTRA_LAZY associations around a LazyCriteria. However, ManyToMany are still completely loaded: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/PersistentCollection.php#L874

This is still problematic from a performance point of view because count, contains... cannot be optimized. I think the solution is similar to previous one, hence creating a Lazy collection for that kind of associations.

However, this is really tricky to do because of the whole mess inside the persisters (can't wait for them to be completely refactored, it's getting really hard to maintain this mess :p).

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3130

We use Jira to track the state of pull requests and the versions they got
included in.

@bakura10
Copy link
Member Author

Just curious: as custom persisters is not supported, how hard would it be to refactor the persisters completely (as there is no problematic of BC in this part of the code then)?

@Ocramius
Copy link
Member

@bakura10 probably not a big deal

@@ -254,6 +258,11 @@ public function count(PersistentCollection $coll)
. ' FROM ' . $joinTableName . ' t'
. $joinTargetEntitySQL
. ' WHERE ' . implode(' AND ', $conditions);
var_dump($joinTargetEntitySQL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry some debugging of mine.

Actually I think I'll need to rethink this PR. I think I can actually reuse the various count, slice, exists... method of the persistentCollection. The problem is that because of the additional criteria, I should make those various methods aware of the Criteria...

For now I'll let this as a WIP and try to come up for a solution if I have some time. But really, I think all those part should be rewritten from scratch...

@bakura10
Copy link
Member Author

bakura10 commented Jul 8, 2014

@stof and @Ocramius : this is not blocking for 2.5 imho, but that would be really nice to have it!

@guilhermeblanco
Copy link
Member

Working on this issue... will require some deep Persisters refactoring.

@Ocramius Ocramius changed the title [WIP] Lazy criteria for ManyToMany collection Lazy criteria for ManyToMany collection Jan 24, 2015
@Ocramius
Copy link
Member

Tried to integrate this PR in the existing persisters, and I realized that parameter types are not considered.

@nick4fake
Copy link

Any news on this?

@Inwerpsel
Copy link

Inwerpsel commented Oct 1, 2018

Any news on this? I understand that it looks like an edge case, however it really prevents anyone implementing EXTRA_LAZY associations from really benefitting from them.

If no fix is upcoming, can someone maybe explain what the difficulty in implementing this exactly is?

Base automatically changed from master to old-prototype-3.x February 23, 2021 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants