Validate filter and queryVector parameter in $vectorSearch stage builder#2857
Validate filter and queryVector parameter in $vectorSearch stage builder#2857GromNaN merged 1 commit intodoctrine:2.13.xfrom
filter and queryVector parameter in $vectorSearch stage builder#2857Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds validation to the $vectorSearch stage builder to provide early feedback for invalid parameters and extends the filter option to accept arrays in addition to expressions.
- Adds validation for
queryVectorparameter to ensure Binary types use the correct vector type and arrays are lists rather than associative arrays - Extends the
filtermethod to accept both arrays and Expr objects instead of only Expr objects - Adds comprehensive test coverage for the new validation logic
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/Doctrine/ODM/MongoDB/Aggregation/Stage/VectorSearch.php | Implements validation logic for queryVector and extends filter method to accept arrays |
| tests/Doctrine/ODM/MongoDB/Tests/Aggregation/Stage/VectorSearchTest.php | Adds test cases for array filter usage and queryVector validation |
| phpstan-baseline.neon | Adds PHPStan baseline entry for the new mixed array type in filter property |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
GromNaN
force-pushed
the
validate-binary-vector
branch
from
f8a8073 to
83b7a0e
Compare
GromNaN
force-pushed
the
validate-binary-vector
branch
from
83b7a0e to
99ba67a
Compare
|
|
||
| #[TestWith([new Binary("\x03\x00\x01\x02\x03", Binary::TYPE_GENERIC), 'Binary query vector must be of type 9 (Vector), got 0.'])] | ||
| #[TestWith([[1 => 1, 2 => 3], 'Query vector must be a list of numbers, got an associative array.'])] | ||
| #[TestWith([[], 'Query vector cannot be an empty array.'])] |
There was a problem hiding this comment.
Oh cool, I had not seen these attributes before.
There was a problem hiding this comment.
Before attributes, it was the @testWith annotation but the syntax was awful.
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
A couple of questions which might lead to some small changes, but defer to you.
| private ?int $numCandidates = null; | ||
| private ?string $path = null; | ||
| /** @see Binary::TYPE_VECTOR introduced in ext-mongodb 2.2 */ | ||
| private const BINARY_TYPE_VECTOR = 9; |
There was a problem hiding this comment.
Just to confirm, will this stick around even after the ext-mongodb 2.2.0 release? I assume you don't want to raise the driver dependency just for this feature.
There was a problem hiding this comment.
Exactly, we don't want to update the required version for this feature. But someone could receive a binary vector from the server and use in a query. That's why we have this constant duplicated here.
| } | ||
|
|
||
| if (is_array($queryVector) && ! array_is_list($queryVector)) { | ||
| throw new InvalidArgumentException('Query vector must be a list of numbers, got an associative array.'); |
There was a problem hiding this comment.
Do you actually intend to validate the contents of the list, or will you rely on the server to report that error?
There was a problem hiding this comment.
Yes, this is a partial validation. I don't know what vector type is expected so iterating over the values would just cost time.
Summary
queryVector.