Skip to content

Comments

Add support for using $text operator in a aggregation with a filter#2739

Merged
GromNaN merged 2 commits intodoctrine:2.11.xfrom
GromNaN:gh-2733
Mar 5, 2025
Merged

Add support for using $text operator in a aggregation with a filter#2739
GromNaN merged 2 commits intodoctrine:2.11.xfrom
GromNaN:gh-2733

Conversation

@GromNaN
Copy link
Member

@GromNaN GromNaN commented Feb 28, 2025

Q A
Type bug
BC Break no
Fixed issues Fix #2733

Summary

The $text operator must be in the first $match stage. Instead of prepending the pipeline with an other $match stage for the filter, the builder merge them using a logical $and.

We cannot detect when $text is used as this can be deep in the $match query, so the behavior is for all the pipelines.

@GromNaN GromNaN added this to the 2.11.0 milestone Feb 28, 2025
@GromNaN GromNaN requested a review from jmikola March 3, 2025 15:38
@GromNaN GromNaN added Bug and removed Bug labels Mar 3, 2025
@GromNaN GromNaN requested a review from jmikola March 3, 2025 18:55
if (isset($pipeline[0]['$match'])) {
// Merge filters into the first $match stage with a logical $and
$pipeline[0]['$match'] = [
'$and' => [
Copy link
Member

@jmikola jmikola Mar 3, 2025

Choose a reason for hiding this comment

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

Starting a separate comment thread for this. Apologies for not asking this sooner, but do you know why the $geoNear stage is able to use:

$this->applyFilters($pipeline[0]['$geoNear']['query']);

Since that merges the filter and discriminator criteria into the query, I'm curious as to why we could do the following instead of $and:

$pipeline[0]['$match'] = $this->applyFilters($pipeline[0]['$match']);

Copy link
Member Author

@GromNaN GromNaN Mar 3, 2025

Choose a reason for hiding this comment

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

Excellent point, I probably didn't go far enough in my exploration. In fact, CriteriaMerger already does the job very well.

public function merge(...$criterias): array
{
$nonEmptyCriterias = array_values(array_filter($criterias, static fn (array $criteria) => ! empty($criteria)));
switch (count($nonEmptyCriterias)) {
case 0:
return [];
case 1:
return $nonEmptyCriterias[0];
default:
return ['$and' => $nonEmptyCriterias];
}
}

I modifies the implementation to match with $geoNear. Good news, I didn't have to touch the tests and it cleanly drops empty $match criteria.

Copy link
Member Author

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

It's getting bigger than expected, but that's no bad thing.

Comment on lines +75 to +76
$stage = ['near' => [0, 0], 'spherical' => false, 'distanceField' => null, 'query' => (object) [], 'num' => 1];
self::assertEquals([['$geoNear' => $stage]], $builder->getPipeline());
Copy link
Member Author

Choose a reason for hiding this comment

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

This was an error: $geoNear.query does not accept an empty array

MongoServerError[TypeMismatch]: query must be an object

Copy link
Member

Choose a reason for hiding this comment

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

Does this suggest that the geoNear() builder method never worked in previous versions when query criteria was omitted? That seems like something worthy of its own ticket for changelog visibility, even if you want to keep the fix in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Explicitly fixed in #2743.

$query = $documentPersister->addFilterToPreparedQuery($query);

return $query;
return $query === [] ? (object) $query : $query;
Copy link
Member Author

Choose a reason for hiding this comment

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

I have to juggle between array and empty stdClass to ensure that we have a map when converting to BSON.

}

if ($this->getStage(0) instanceof Stage\GeoNear) {
if (isset($pipeline[0]['$geoNear'])) {
Copy link
Member Author

@GromNaN GromNaN Mar 3, 2025

Choose a reason for hiding this comment

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

This is more efficient and more resilient if we introduce changes on the $pipeline array like #2740.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

You can decide if it's worth reporting a separate issue for the outstanding geoNear() bug.

Comment on lines +75 to +76
$stage = ['near' => [0, 0], 'spherical' => false, 'distanceField' => null, 'query' => (object) [], 'num' => 1];
self::assertEquals([['$geoNear' => $stage]], $builder->getPipeline());
Copy link
Member

Choose a reason for hiding this comment

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

Does this suggest that the geoNear() builder method never worked in previous versions when query criteria was omitted? That seems like something worthy of its own ticket for changelog visibility, even if you want to keep the fix in this PR.

'stype' => 'server_guest',
'$text' => ['$search' => 'Paul'],
],
['filtered' => true],
Copy link
Member

Choose a reason for hiding this comment

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

The query here seems odd, but I understand that's because addDiscriminatorToPreparedQuery() sets the key directly and addFilterToPreparedQuery() uses CriteriaMerger, which will always adds $and for 2+ criteria.

@GromNaN GromNaN changed the title Add support for using $text operator in a aggregation with a filter Add support for using $text operator in a aggregation with a filter and fix $geoNear stage with empty query Mar 4, 2025
@GromNaN GromNaN changed the title Add support for using $text operator in a aggregation with a filter and fix $geoNear stage with empty query Add support for using $text operator in a aggregation with a filter Mar 5, 2025
@GromNaN GromNaN merged commit b0252b6 into doctrine:2.11.x Mar 5, 2025
19 checks passed
@GromNaN GromNaN deleted the gh-2733 branch March 5, 2025 09:40
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.

2 participants