-
Notifications
You must be signed in to change notification settings - Fork 209
PHPC-2474: Support Binary vector subtype #1853
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the python implementation, the Binary
class has a from_vector
static method. You have certainly considered this solution, of having only a factory method instead of a new class. But the new methods toArray
and getVectorType
are specific to this binary type, so having a dedicated class makes sense. But if you create a new Binary($raw, Binary::TYPE_VECTOR)
instance with the vector type from the raw binary data, you don't have this methods. The current state doesn't allow creating a BinaryVector
from raw data. I think you would have to wrap in into a BSON Document and encode/decode it to get a BinaryVector
instance. I don't find any other binary type that would require a specific class.
I see 3 possible implementations:
- Add
Binary::fromVector(array $vector, VectorType $vectorType)
andtoArray()
to the existingBinary
class CreateBinaryVector
class that extendsBinary
(current state of this PR).CreateBinaryVector
class that implementsBinaryInterface, \JsonSerializable, Type, \Stringable
.
We need to compare pro/con of each solution.
7843292
to
3bf3458
Compare
726240f
to
6ffd63d
Compare
4df67fd
to
1649670
Compare
One failing test appears to be from Atlas connectivity tests failing to connect to a serverless cluster. Per PHPLIB-1667, I expect that should have been removed; however, I don't see a related PHPC ticket. Please review ignoring that particular test failure and I'll address that with a separate PR and ticket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No objection to adding corpus tests in a follow-up PR.
22ddb35
to
acc9413
Compare
@@ -0,0 +1,18 @@ | |||
--TEST-- | |||
MongoDB\BSON\Binary unserialization requires valid vector data (__serialize and __unserialize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this complements bson-binary-set_state_error-004.phpt
acc9413
to
0c56e9f
Compare
0c56e9f
to
6bd8a44
Compare
https://jira.mongodb.org/browse/PHPC-2474