Improve LazyCriteriaCollection matching

[TheDutchScorpion]

This will prevent initializing when matching on a LazyCriteriaCollection and will keep the Collection lazy.

[TheDutchScorpion]

I opened a PR within the collection repository for merging two Criteria together Feature Criteria merge. The uninitialized matching return could be changed to return new self($this->entityPersister, $this->criteria->merge($criteria)); after merged.

[greg0ire]

I think this deserves more explanation, because with your changes, I think calling matching() on the same collection object twice would result in 2 SQL queries instead of one. That's a performance regression, right? So maybe it improves performance in some scenarios (which would qualify as an improvement, not as a fix, and means you should target 2.15.x), but I think it might degrade it in some other scenarios.

[TheDutchScorpion]

Calling matching() will merge the criteria together and create a new lazy instance with the merged criteria.
When the lazy collection is already initialized this collection will be used, instead of merging the criteria. This way there will only be a SQL query executed when the collection is really initialized.

[greg0ire]

Okay, but what it is not initialized?
What happens if I call matching() on the same uninitialized $collection object twice?

[TheDutchScorpion]

Okay, but what it is not initialized? What happens if I call matching() on the same uninitialized $collection object twice?

That will result in two new lazy collection instances that are not initialized with the merged Criteria from the matched collection and no SQL queries are executed, unless you initialize both collection that will result in 2 SQL queries.

[greg0ire]

Well if you get 2 collections, chances are, you are going to do things with them eventually. If that's the case, you will initialize them, and end up with 2 SQL queries where there was just one before. Let me know if I'm missing something.

[TheDutchScorpion]

In one of my projects I'm having a entity with a large OneToMany relation that is set to EXTRA_LAZY fetching. When calling matching on this relation no SQL queries are executed, but when I'm calling matching again everything initialized while I only need a small portion of the collection. This action is very slow and uses a lot of memory.

With the code in this PR when I call matching on the OneToMany everything will be the same, but the second matching will return a LazyCriteriaCollection when not initialized, otherwise a ArrayCollection will be returned.
This means that when calling one of the methods that trigger a initialize before the matching a ArrayCollection will be return and no SQL queries are executed, because all collection elements are already loaded.

[derrabus]

I mean, I happily re-approve the CI workflow again and again, but you do know you can run all those checks locally, right? 😓

[greg0ire]

@TheDutchScorpion thanks for the explanation

when I'm calling matching again everything

Are you calling it on the relation again, or are you calling it on the object returned by the first call to matching()? If you can write some pseudo-code for this it might help me understand (I'm not very familiar with this stuff).

[TheDutchScorpion]

Are you calling it on the relation again, or are you calling it on the object returned by the first call to matching()?

I'm calling it on the Collection returned by the first matching().

[TheDutchScorpion]

Old situation

$bar = 'bar';
$collection = LazyCriteriaCollection;
$newCollection = $collection->matching(Criteria::expr()->eq('foo', $bar)); // $collection will be initialized after matching and returns initialized ArrayCollection
$newCollection->matching(Criteria::expr()->eq('foo', $bar));
$newCollection->getIterator();

New situation

$bar = 'bar';
$collection = LazyCriteriaCollection;
$newCollection = $collection->matching(Criteria::expr()->eq('foo', $bar)); // $collection will not be initialized and returns LazyCriteriaCollection with merged criteria
$newCollection->matching(Criteria::expr()->eq('foo', $bar));
$newCollection->getIterator(); // $newCollection will initialize and will perform the same as in the old situation

[greg0ire]

Ok, I think I fully get it now: this postpones the SQL query to getIterator(), while in the previous situation the initialization was done in matching. My concern about calling matching() twice on an unitialized collection still stands I think, because then you end up with 2 different objects, while previously you had the same object.

$bar = 'bar';
$collection = LazyCriteriaCollection;
$newCollection = $collection->matching(Criteria::expr()->eq('foo', $bar)); // SQL query here
// … later on
$newCollection = $collection->matching(Criteria::expr()->eq('foo', $bar)); // also an SQL query here

If that's accurate I'm not sure we can merge this. WDYT @derrabus @SenseException ?

[TheDutchScorpion]

Maybe it would be an idea to create another fetch type for this, because in some situations you don't want too fetch everything before matching on it.

[SenseException]

If this is going to be merged then I wouldn't to it in 2.15, maybe not even in 2.x. This change in behavior may affect performance for other people negatively, as mentioned before, and I don't see a major advantage in having this behavior in ORM.

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[github-actions]

This pull request was closed due to inactivity.