Add pipeline to $lookup stage #2459
Conversation
|
I discovered that "foreignField" and "localField" is not mandatory when using "pipeline" in the $lookup stage. In fact it dropped the performance in our case so we had a query running for about 4 seconds. Without foreignField and localField the query runs in 250-300ms and the result set was the same. |
|
@Ravetracer Thanks for your comment. Changes pushed to PR. |
|
@khaperets thanks for the PR! It seems that the test you've added is failing: https://github.com/doctrine/mongodb-odm/actions/runs/3067090011/jobs/4971970883#step:12:1783. The rest of errors seems to be unrelated, we'll investigate them. Once the test is green please fix coding standards, this can be done by running |
|
@malarzm I've done everything, thank you for the feedback. |
|
@khaperets we've managed to get checks back to shape. May I ask you to rebase your PR atop newly created |
This tests started to fail with the 1.14 version of the driver
We were testing the annotation instantiation which is done by doctrine/annotations package.
|
@malarzm done |
| if (! empty($this->let)) { | ||
| $expression['$lookup']['let'] = $this->let; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
According to https://www.mongodb.com/docs/manual/reference/operator/aggregation/lookup/#correlated-subqueries-using-concise-syntax MongoDB 5.0 allows using all 4 fields together so the current these-or-those approach doesn't seem like a good idea.
Add pipeline to $lookup stage
Add pipeline to $lookup stage
|
@malarzm Added compatibility with MongoDB 5.0 |
| 'as' => $this->as, | ||
| ], | ||
| ]; | ||
|
|
||
| $serverVersion = $this->dm->getServerVersion($this->class->getName()); | ||
| $isSupportsPipelineWithLocalField = version_compare($serverVersion, '5.0') >= 0; |
There was a problem hiding this comment.
Generally we're letting end users handle version-specific behaviour as they do know what MongoDB version they're running. The most important reason is to prevent additional communication with database, as introduced here. So instead I'd rather see a way to reset builder's localField and foreignField properties when adding pipeline/let. I think the cleanest way to do will be a new method resetLocalAndForeignField that will unset both properties. Additionally this will allow use cases like #2459 (comment) to control how they query will be ran.
Also I didn't manage to comment last time, but I believe I've seen the test fail due to MongoDB version. Once we get back to letting users control the behaviour, you can skip some tests using methods like \Doctrine\ODM\MongoDB\Tests\BaseTest::requireMongoDB42.
Also feel free to ask me anything on our Slack, that way I'll be able to help faster :)
|
Merged manually in 7ebc038. Thanks a lot @khaperets! |
Summary
Added a pipeline lookup functionality that helps a lot in the futures developments,