CSHARP-5202: BSON Binary Vector Subtype Support #1581

BorisDog · 2024-12-27T00:28:41Z

No description provided.

papafe

Overall it makes sense, I just got some notes/questions

src/MongoDB.Bson/ObjectModel/BsonVector.cs

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

Co-authored-by: Ferdinando Papale <[email protected]>

src/MongoDB.Bson/ObjectModel/VectorDataType.cs

sanych-sun · 2025-01-20T23:38:49Z

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

+            {
+                var itemType = type.GetElementType();
+                var vectorSerializerType = typeof(BsonVectorArraySerializer<>).MakeGenericType(itemType);
+                var vectorSerializer = (IBsonSerializer)Activator.CreateInstance(vectorSerializerType, DataType);


I would suggest to move this into BsonVectorArraySerializer.Create(Type itemType) static factory method.

Here and more similar below.

It's not possible as BsonVectorArraySerializer and the rest are generic classes.

But we can create non-generic static class just to have this factory methods. We are doing it in other places. (far example, NullableSerializer)

src/MongoDB.Bson/ObjectModel/VectorDataType.cs

sanych-sun · 2025-01-20T23:58:43Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    /// </summary>
+    /// <typeparam name="TItemCollection">The concrete type derived from <see cref="BsonVector{T}"/>.</typeparam>
+    /// <typeparam name="TItem">The .NET data type.</typeparam>
+    public sealed class BsonVectorSerializer<TItemCollection, TItem> : BsonVectorSerializerBase<TItemCollection, TItem>


Should we override Equals for the Serializer to make proper comparison of serializers? It might be important if Vestors could be used in LINQ statements.

Let's discuss if it's safe to have comparison implemented in base class only.

We don't override Equals if no private data was introduced.

sanych-sun · 2025-01-21T00:04:26Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+        public static byte[] WriteVectorData<T>(ReadOnlySpan<T> vectorData, BsonVectorDataType bsonVectorDataType, byte padding)
+            where T : struct
+        {
+            var vectorDataBytes = MemoryMarshal.Cast<T, byte>(vectorData);


I suppose we should ensure we are allowed to do that and throw in case wrong endian-platform.

Great catch. Done.

sanych-sun · 2025-01-21T00:07:52Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+{
+    internal static class BsonVectorWriter
+    {
+        public static byte[] WriteBsonVector<T>(BsonVector<T> bsonVector)


Minor: The name of the method is a little confusing to me. If it's WriteXXX I expect to pass target into the method, like a stream of buffer where I want it to be written to. In this case, I think FormatXXX and ParseXXX would work better instead of Write and Read.

Good point. Done.

- PR comments

BorisDog

Add support for VectorSearch.
Test cluster setup is WIP.

src/MongoDB.Bson/ObjectModel/VectorDataType.cs

BorisDog · 2025-01-24T23:53:23Z

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

+            {
+                var itemType = type.GetElementType();
+                var vectorSerializerType = typeof(BsonVectorArraySerializer<>).MakeGenericType(itemType);
+                var vectorSerializer = (IBsonSerializer)Activator.CreateInstance(vectorSerializerType, DataType);


It's not possible as BsonVectorArraySerializer and the rest are generic classes.

BorisDog · 2025-01-24T23:55:25Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+{
+    internal static class BsonVectorWriter
+    {
+        public static byte[] WriteBsonVector<T>(BsonVector<T> bsonVector)


Good point. Done.

BorisDog · 2025-01-24T23:56:28Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+        public static byte[] WriteVectorData<T>(ReadOnlySpan<T> vectorData, BsonVectorDataType bsonVectorDataType, byte padding)
+            where T : struct
+        {
+            var vectorDataBytes = MemoryMarshal.Cast<T, byte>(vectorData);


Great catch. Done.

BorisDog · 2025-01-24T23:58:47Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    /// </summary>
+    /// <typeparam name="TItemCollection">The concrete type derived from <see cref="BsonVector{T}"/>.</typeparam>
+    /// <typeparam name="TItem">The .NET data type.</typeparam>
+    public sealed class BsonVectorSerializer<TItemCollection, TItem> : BsonVectorSerializerBase<TItemCollection, TItem>


…-driver into csharp5202

- PR comments

Co-authored-by: Ferdinando Papale <[email protected]>

sanych-sun · 2025-01-27T20:42:20Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    /// </summary>
+    /// <typeparam name="TItemCollection">The collection type.</typeparam>
+    /// <typeparam name="TItem">The .NET data type.</typeparam>
+    public abstract class BsonVectorSerializerBase<TItemCollection, TItem> : SerializerBase<TItemCollection>


Should we move each serializer into it's own file?

The nested classes are very simple just a few lines long. I find it more readable to keep all in the same file in this case. But we can discuss further.

I think we have often bundled very tightly related classes together in a single file. I'm fine with this.

sanych-sun · 2025-01-27T20:42:52Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    /// </summary>
+    /// <typeparam name="TItemCollection">The concrete type derived from <see cref="BsonVector{T}"/>.</typeparam>
+    /// <typeparam name="TItem">The .NET data type.</typeparam>
+    public sealed class BsonVectorSerializer<TItemCollection, TItem> : BsonVectorSerializerBase<TItemCollection, TItem>


Let's discuss if it's safe to have comparison implemented in base class only.

- Rebase

rstam

Overall looks good. But I have a lot of naming suggestions and a few refactoring suggestions. Let me know what you think.

rstam · 2025-01-29T18:13:00Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+    /// <summary>
+    /// Represents a BSON vector.
+    /// </summary>
+    public abstract class BsonVectorBase<T>


I think a better name for this class is BsonVector (no Base needed in the name).

Yeah that's what I've started with, but then I realized that there is some ambiguity with BsonVectorAttribute:
When trying to define the following poco without including using MongoDB.Bson.Serialization.Serializers; namespace, user unclear error without clear suggested resolution.

class POCO { [BsonVector(...)] public int[] Vector { get; set; } }

I think [BsonVector(...)] should be [BsonVectorRepresentation(...)] which would eliminate this conflict.

A similar class is BsonValue which is the abstract base class for multiple types of BSON values.

Here BsonVector is the base class for multiple types of BSON vectors.

Note that we did not name BsonValue as BsonValueBase.

I agree that without considering BsonVectorAttribute, BsonVector is more natural name.
But I still think it's preferable to reserve BsonVector for BsonVectorAttribute, as it's expected to appear in most of the use cases, while direct usages of BsonVectorBase are expected to be rare.

rstam · 2025-01-29T18:17:35Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+        /// <summary>
+        /// Gets the vector.
+        /// </summary>
+        public ReadOnlyMemory<T> Vector { get; }


The spec calls this property Data.

rstam · 2025-01-29T18:18:17Z

src/MongoDB.Bson/ObjectModel/VectorDataType.cs

+    /// Represents the data type of BSON Vector.
+    /// </summary>
+    /// <seealso cref="BsonVectorBase{T}"/>
+    public enum BsonVectorDataType


Rename file to match enum name.

Thanks, done.

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

rstam · 2025-01-29T18:20:24Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+
+namespace MongoDB.Bson.Serialization.Serializers
+{
+    internal static class BsonVectorSerializerBase


I think this static class should be named just BsonVectorSerializer (no Base in the name).

rstam · 2025-01-29T18:37:36Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+            return (elements, padding, vectorDataType);
+        }
+
+        public static (ReadOnlyMemory<byte> VectorDataBytes, byte Padding, BsonVectorDataType VectorDataType) ReadBsonVectorAsBytes(ReadOnlyMemory<byte> vectorData)


Is As redundant in the name? Could be ReadBsonVectorBytes or ReadBsonVectorData.

VectorDataBytes could be shortened to Bytes if you want.

rstam · 2025-01-29T18:38:03Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+            var vectorDataSpan = vectorData.Span;
+            var vectorDataType = (BsonVectorDataType)vectorDataSpan[0];
+
+            var paddingSizeBits = vectorDataSpan[1];


This variable could be named just padding.

rstam · 2025-01-29T18:38:39Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+            return (vectorData.Slice(2), paddingSizeBits, vectorDataType);
+        }
+
+        private static BsonVectorBase<T> CreateBsonVector<T>(T[] elements, byte padding, BsonVectorDataType vectorDataType)


Should this be a static factory method in the BsonVector<T> class?

I don't think so: I didn't see a need for a such a factory outside this class. Also I think BsonVector itself should be just a data holder, decoupled from its factories.

Rename T to TItem for consistency.

rstam · 2025-01-29T18:39:24Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+{
+    internal static class BsonVectorWriter
+    {
+        public static byte[] BsonVectorToBytes<T>(BsonVectorBase<T> bsonVector)


Consider naming this method WriteBytes or WriteToBytes.

The "from" is implied by the parameter types.

rstam · 2025-01-29T18:39:45Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+            return VectorDataToBytes(bsonVector.Vector.Span, bsonVector.DataType, padding);
+        }
+
+        public static byte[] VectorDataToBytes<T>(ReadOnlySpan<T> vectorData, BsonVectorDataType bsonVectorDataType, byte padding)


Consider naming WriteToBytes.

- PR comments

rstam

Sorry I have so many requested naming changes.

But specially where things are entering the public API it is important to get the names right because we can't change them later.

rstam · 2025-01-30T21:27:56Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+    /// <summary>
+    /// Represents a BSON vector.
+    /// </summary>
+    public abstract class BsonVectorBase<T>


I think [BsonVector(...)] should be [BsonVectorRepresentation(...)] which would eliminate this conflict.

rstam · 2025-01-30T21:28:10Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+        /// <summary>
+        /// Initializes a new instance of the BsonVector class.
+        /// </summary>
+        public BsonVectorBase(ReadOnlyMemory<T> data, BsonVectorDataType dataType)


protected internal?

I think <T> should be <TItem> for consistency and as a hint of what T actually is.

rstam · 2025-01-30T21:33:17Z

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

+    /// Sets the representation for this field or property as BSON Vector and specifies the serialization options.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field)]
+    public sealed class BsonVectorAttribute : Attribute, IBsonMemberMapAttribute


I think this attribute should be named BsonVectorRepresentationAttribute.

When we use:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] public byte[] A { get; set; }

what we are saying is that A is REPRESENTED as a BsonVector, not that it IS a BsonVector (it's not, it's a byte[]).

Other similarly named attributes that are used to control representation are BsonRepresentation and BsonGuidRepresentation.

Unfortunately there are some other attributes where we neglected to follow this naming principle. :(

I understand your argument that you wanted a short name for the attribute, but optimal names are not just about length, after all if that were true why not this:

[BV(BVDT.I8)]

I think having Representation in the attribute name makes it self explanatory that we are configuring the representation of A.

Or maybe it's a BsonVector, just represented by byte[] :)

I think that within the given context, it's clear that [BsonVector(...)] refers to the bson representation of the property on the wire. I don't think that adding "Representation" actually adds any addition information. In fact I see BsonVectorAttribute closer to BsonSerializerAttribute.

BsonGuidRepresentation on the other hand switches between actual representation of the guid and BsonRepresentation switches between different bson representations. Which is not the case with BsonVector, which can be viewed as a private case of BsonRepresentation, like a hypothetical "BsonDoubleAttribute.

It even can argued that [BsonVectorRepresentation(BsonVectorDataType.Int8)] is more confusing. It might sound that this attribute configures the representation of the field when serialized as BsonVector type and leaves the question of the actual bson type open. For example user could expect to have:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] [BsonArrayRepresentation(BsonType.Binary)] public byte[] A { get; set; }

Or maybe it's a BsonVector, just represented by byte[] :)

Yes but... serializers have NO control over how things are represented in memory. That is totally defined by the C# data type that this is a serializer for (in this case byte[]). In the context of configuring serialization "representation" ALWAYS means how things are represented IN THE DATABASE.

In this case the in memory representation is byte[] and the database representation is BsonVector. The only thing that can be configured is the database representation.

I think the disconnect here is that I am looking at this from the perspective of "configuring the representation of the C# value in the database" and you are coming from the opposite direction. But as I said, in the case of serializer configuration it is always the database representation that is being configured.

Since I'm thinking about this in terms of "configuring serialization" which in turn means "configuring the database representation" that is why to me [BsonVector(...)] by itself makes no sense.

Regarding:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] [BsonArrayRepresentation(BsonType.Binary)] public byte[] A { get; set; }

the problem of a user applying contradictory attributes to the same member is not one we have solved.

Also, BsonArrayRepresentation doesn't exist but I guess you are using it as a hypothetical.

I'm going to think about this some more...

What I want is a name that clearly implies "A is represented in the database as a BsonVector".

And not one that reads like "A is a BsonVector", because it is not.

I guess the word "representation" is relevant to most of our bson attributes and config options. So it becomes redundant at some point. For example BsonTimeSpanOptions can be thought as BsonTimeSpanRepresentation, as it does configure how the BsonTime is represented.

I don't think having BsonVectorRepresentation is entirely wrong, just not very accurate. We are configuring the bson representation of the clr type, so the ideal name probably would be BsonRepresentation(vectorSubtype, vectorType, ...), and we are not configuring just the vector representation of the clr type.

I think BsonVector option is just more concise and easier to use. Especially given that bson vector is a new special type with a very specific use cases.

I really have not been able to find my way to be comfortable with your choice of BsonVectorBase and [BsonVector(...)].

BsonVectorBase is just wrong, and you didn't choose it because you liked it, you chose it because the attribute name of [BsonVector(...)] created a partial conflict with it. I think we just need to rename the attribute instead of the BsonVector base class.

BsonVector "looks" like a type name just like BsonString (and it should be a type name instead of BsonVectorBase).

Using this:

[BsonVector(BsonVectorDataType.Int8)] public byte[] V;

is analogous to the following invalid code:

[BsonString] // BsonString is analogous in this position to "BsonVector" in the previous example public Enum E;

which "looks" like it says "E is a BsonString", when what we want to say is "E is represented as a BsonString", which we do like this:

[BsonRepresentation(BsonType.String)] public Enum E;

I tried a thought experiment to how I would design configuring representing things as a BsonVector. The first thought I had that matches our existing attributes is:

[BsonRepresentation(BsonType.Binary, BsonSubType.Vector, BsonVectorDataType.Int8)] public byte[] A;

which would be a new overload of BsonRepresentation. While that is accurate it's a bit redundant and could be shortened to by implying the binary subtype 9:

[BsonRepresentation(BsonVectorDataType.Int8)] public byte[] A;

Since I thought you may not want to overload the existing BsonRepresentation I changed that to:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] public byte[] A;

Which is the same as your proposal except for the name:

[BsonVector(BsonVectorDataType.Int8)] public byte[] A;

I have two big problems with [BsonVector(...)]:

It reads like "A is a BsonVector" instead of "A is represented as a BsonVector"

It creates the pseudo conflict with using BsonVector as a class name (hence your awkward BsonVectorBase class name)

I think my proposal:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] public byte[] A;

Is the best solution because it makes clear that we are configuring the representation of A to be a BsonVector. But I am open to other names as long as it's not BsonVector.

The concept I'm trying to capture is:

[RepresentedAsABsonVectorOf(BsonVectorDataType.Int8)] public byte[] A;

or

[SerializedAsABsonVectorOf(BsonVectorDataType.Int8)] public byte[] A;

so maybe we can start from there to find a new attribute name that is not [BsonVector].

If we can't agree on a new name I would reluctantly LGTM this PR with strong objections to this name because everything else is looking good to me.

I have a further proposal.

Instead of:

[BsonVector(BsonVectorDataType.Int8)] public byte[] ValuesByte { get; set; }

we could do this:

[SerializedAsInt8Vector] public byte[] ValuesByte { get; set; }

This is shorter, simpler, and includes the important semantic information that exactly what is happening here is that we are configuring the property to be "serialized as an Int8 vector".

Simlarly for [SerializedAsPackedBitVector] or [SerializedAsFloat32Vector]

rstam · 2025-01-30T21:35:27Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    /// </summary>
+    /// <typeparam name="TItemCollection">The collection type.</typeparam>
+    /// <typeparam name="TItem">The .NET data type.</typeparam>
+    internal abstract class BsonVectorSerializerBase<TItemCollection, TItem> : SerializerBase<TItemCollection>


I am confused by "collection" in the name TItemCollection because it implies that the type implements ICollection or at least IEnumerable but that's only true sometimes.

Can we use a more neutral term like "container" and name this TItemContainer?

rstam · 2025-01-30T21:36:05Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    }
+
+    /// <summary>
+    /// Represents a serializer for BSON vector to/from a given collection.


/// Represents a serializer for TItemContainer values represented as a BsonVector.

The part immediately after for should be what is being serializer, not how it is represented.

rstam · 2025-01-30T21:52:04Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+            }
+        }
+
+        public static void ValidateDataType<T>(BsonVectorDataType bsonVectorDataType)


Rename T to TItem for consistency.

rstam · 2025-01-30T21:52:42Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+                _ => throw new ArgumentOutOfRangeException(nameof(bsonVectorDataType), bsonVectorDataType, "Unsupported vector datatype.")
+            };
+
+            if (supportedType != typeof(T))


I think this would read more logically swapping the sides:

if (typeof(TItem) != supportedType)

That's how you would say it in English.

I look at it from a different perspective "What is the value of my variable", similar to if variable == constant.
Also typeof(T) is in a way constant here.

rstam · 2025-01-30T21:53:13Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+{
+    internal static class BsonVectorWriter
+    {
+        public static byte[] WriteToBytes<T>(BsonVectorBase<T> bsonVector)


Rename T to TItem for consistency.

rstam · 2025-01-30T21:53:17Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+            return WriteToBytes(bsonVector.Data.Span, bsonVector.DataType, padding);
+        }
+
+        public static byte[] WriteToBytes<T>(ReadOnlySpan<T> vectorData, BsonVectorDataType bsonVectorDataType, byte padding)


Rename T to TItem for consistency.

rstam · 2025-01-30T21:54:45Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+    /// <summary>
+    /// Represents a BSON vector.
+    /// </summary>
+    public abstract class BsonVectorBase<T>


A similar class is BsonValue which is the abstract base class for multiple types of BSON values.

Here BsonVector is the base class for multiple types of BSON vectors.

Note that we did not name BsonValue as BsonValueBase.

BorisDog

Thanks for the thorough review. I think it's much more polished now.

BorisDog · 2025-01-31T00:15:26Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+    /// <summary>
+    /// Represents a BSON vector.
+    /// </summary>
+    public abstract class BsonVectorBase<T>


I agree that without considering BsonVectorAttribute, BsonVector is more natural name.
But I still think it's preferable to reserve BsonVector for BsonVectorAttribute, as it's expected to appear in most of the use cases, while direct usages of BsonVectorBase are expected to be rare.

BorisDog · 2025-01-31T00:16:43Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+        /// <summary>
+        /// Initializes a new instance of the BsonVector class.
+        /// </summary>
+        public BsonVectorBase(ReadOnlyMemory<T> data, BsonVectorDataType dataType)


BorisDog · 2025-01-31T00:34:43Z

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

+    /// Sets the representation for this field or property as BSON Vector and specifies the serialization options.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field)]
+    public sealed class BsonVectorAttribute : Attribute, IBsonMemberMapAttribute


Or maybe it's a BsonVector, just represented by byte[] :)

I think that within the given context, it's clear that [BsonVector(...)] refers to the bson representation of the property on the wire. I don't think that adding "Representation" actually adds any addition information. In fact I see BsonVectorAttribute closer to BsonSerializerAttribute.

BsonGuidRepresentation on the other hand switches between actual representation of the guid and BsonRepresentation switches between different bson representations. Which is not the case with BsonVector, which can be viewed as a private case of BsonRepresentation, like a hypothetical "BsonDoubleAttribute.

It even can argued that [BsonVectorRepresentation(BsonVectorDataType.Int8)] is more confusing. It might sound that this attribute configures the representation of the field when serialized as BsonVector type and leaves the question of the actual bson type open. For example user could expect to have:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] [BsonArrayRepresentation(BsonType.Binary)] public byte[] A { get; set; }

BorisDog · 2025-01-31T00:35:59Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+        /// Initializes a new instance of the <see cref="BsonVectorSerializerBase{TItemCollection, TItem}"/> class.
+        /// </summary>
+        /// <param name="bsonVectorDataType">Type of the bson vector data.</param>
+        public BsonVectorSerializerBase(BsonVectorDataType bsonVectorDataType)


BorisDog · 2025-01-31T00:36:39Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+            context.Writer.WriteBinaryData(binaryData);
+        }
+
+        private static BsonVectorDataType GetDataType() =>


Good find, it's a leftover, thanks!

BorisDog · 2025-01-31T01:04:52Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+        }
+
+        /// <inheritdoc/>
+        public override sealed void Serialize(BsonSerializationContext context, BsonSerializationArgs args, TItemCollection bsonVector)


BorisDog · 2025-01-31T01:05:00Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+        /// <summary>
+        /// Initializes a new instance of the <see cref="BsonVectorToCollectionSerializer{TItemCollection, TItem}" /> class.
+        /// </summary>
+        public BsonVectorToCollectionSerializer(BsonVectorDataType bsonVectorDataType) :


BorisDog · 2025-01-31T01:05:06Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+        }
+
+        /// <inheritdoc/>
+        public override sealed TItemCollection Deserialize(BsonDeserializationContext context, BsonDeserializationArgs args)


BorisDog · 2025-01-31T01:05:29Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+{
+    internal static class BsonVectorWriter
+    {
+        public static byte[] WriteToBytes<T>(BsonVectorBase<T> bsonVector)


BorisDog · 2025-01-31T01:05:33Z

src/MongoDB.Bson/Serialization/BsonVectorWriter.cs

+            return WriteToBytes(bsonVector.Data.Span, bsonVector.DataType, padding);
+        }
+
+        public static byte[] WriteToBytes<T>(ReadOnlySpan<T> vectorData, BsonVectorDataType bsonVectorDataType, byte padding)


rstam · 2025-01-31T22:27:24Z

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

+    /// Sets the representation for this field or property as BSON Vector and specifies the serialization options.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field)]
+    public sealed class BsonVectorAttribute : Attribute, IBsonMemberMapAttribute


I really have not been able to find my way to be comfortable with your choice of BsonVectorBase and [BsonVector(...)].

BsonVectorBase is just wrong, and you didn't choose it because you liked it, you chose it because the attribute name of [BsonVector(...)] created a partial conflict with it. I think we just need to rename the attribute instead of the BsonVector base class.

BsonVector "looks" like a type name just like BsonString (and it should be a type name instead of BsonVectorBase).

Using this:

[BsonVector(BsonVectorDataType.Int8)] public byte[] V;

is analogous to the following invalid code:

[BsonString] // BsonString is analogous in this position to "BsonVector" in the previous example public Enum E;

which "looks" like it says "E is a BsonString", when what we want to say is "E is represented as a BsonString", which we do like this:

[BsonRepresentation(BsonType.String)] public Enum E;

I tried a thought experiment to how I would design configuring representing things as a BsonVector. The first thought I had that matches our existing attributes is:

[BsonRepresentation(BsonType.Binary, BsonSubType.Vector, BsonVectorDataType.Int8)] public byte[] A;

which would be a new overload of BsonRepresentation. While that is accurate it's a bit redundant and could be shortened to by implying the binary subtype 9:

[BsonRepresentation(BsonVectorDataType.Int8)] public byte[] A;

Since I thought you may not want to overload the existing BsonRepresentation I changed that to:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] public byte[] A;

Which is the same as your proposal except for the name:

[BsonVector(BsonVectorDataType.Int8)] public byte[] A;

I have two big problems with [BsonVector(...)]:

It reads like "A is a BsonVector" instead of "A is represented as a BsonVector"

It creates the pseudo conflict with using BsonVector as a class name (hence your awkward BsonVectorBase class name)

I think my proposal:

[BsonVectorRepresentation(BsonVectorDataType.Int8)] public byte[] A;

Is the best solution because it makes clear that we are configuring the representation of A to be a BsonVector. But I am open to other names as long as it's not BsonVector.

The concept I'm trying to capture is:

[RepresentedAsABsonVectorOf(BsonVectorDataType.Int8)] public byte[] A;

or

[SerializedAsABsonVectorOf(BsonVectorDataType.Int8)] public byte[] A;

so maybe we can start from there to find a new attribute name that is not [BsonVector].

rstam · 2025-01-31T22:28:20Z

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

+    /// Sets the representation for this field or property as BSON Vector and specifies the serialization options.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field)]
+    public sealed class BsonVectorAttribute : Attribute, IBsonMemberMapAttribute


If we can't agree on a new name I would reluctantly LGTM this PR with strong objections to this name because everything else is looking good to me.

rstam · 2025-01-31T22:36:30Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+         where TItem : struct
+    {
+        /// <summary>
+        /// Initializes a new instance of the BsonVector class.


/// Initializes a new instance of the BsonVectorBase class.

Although I wish the summary as it stood was correct!

rstam · 2025-01-31T22:36:52Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+        /// <summary>
+        /// Initializes a new instance of the BsonVector class.
+        /// </summary>
+        protected BsonVectorBase(ReadOnlyMemory<TItem> data, BsonVectorDataType dataType)


Should this be internal to prevent users from attempting to create new subclasses?

Good point. Done.

rstam · 2025-01-31T22:37:12Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+        /// </summary>
+        public BsonVectorPackedBit(ReadOnlyMemory<byte> data, byte padding) : base(data, BsonVectorDataType.PackedBit)
+        {
+            if (padding < 0 || padding > 7)


Rider complains that padding can never be less than 0.

Maybe it's time to try Rider, done.

rstam · 2025-01-31T23:03:55Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    }
+
+    /// <summary>
+    /// Represents a serializer for BSON vector to/from <see cref="Memory{TItem}"/>


/// Represents a serializer for <see cref="Memory{TItem}"/> represented as a BsonVector.

rstam · 2025-01-31T23:04:02Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+        private protected override ReadOnlySpan<TItem> GetItemsContainerSpan(TItem[] data) =>
+            data;
+
+        private protected override TItem[] CreateResult(TItem[] elements) =>


private protected override TItem[] CreateResult(TItem[] items) => items;

rstam · 2025-01-31T23:04:29Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+        private protected override ReadOnlySpan<TItem> GetItemsContainerSpan(Memory<TItem> data) =>
+            data.Span;
+
+        private protected override Memory<TItem> CreateResult(TItem[] elements) =>


private protected override Memory<TItem> CreateResult(TItem[] items) => new(items);

rstam · 2025-01-31T23:04:38Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+    }
+
+    /// <summary>
+    /// Represents a serializer for <see cref="ReadOnlyMemory{TItem}"/>.


/// Represents a serializer for <see cref="ReadOnlyMemory{TItem}"/> represented as a BsonVector.

rstam · 2025-01-31T23:04:50Z

src/MongoDB.Bson/Serialization/Serializers/BsonVectorSerializer.cs

+            data.Span;
+
+        private protected override ReadOnlyMemory<TItem> CreateResult(TItem[] elements) =>
+            new(elements);


private protected override ReadOnlyMemory<TItem> CreateResult(TItem[] items) => new(items);

rstam

A few typos remain.

rstam · 2025-02-01T01:13:35Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

+        /// </summary>
+        /// <param name="data">The vector data.</param>
+        /// <param name="dataType">Type of the vector data.</param>
+        internal protected BsonVectorBase(ReadOnlyMemory<TItem> data, BsonVectorDataType dataType)


protected internal was my first thought also, but I looked it up and it's not what I first thought it was.

It should probably just be internal.

rstam · 2025-02-01T01:14:16Z

src/MongoDB.Bson/ObjectModel/BsonVectorDataType.cs

+    /// <summary>
+    /// Represents the data type of a BSON Vector.
+    /// </summary>
+    /// <seealso cref="BsonVectorBase{T}"/>


/// <seealso cref="BsonVectorBase{TItem}"/>

rstam · 2025-02-01T01:15:03Z

src/MongoDB.Bson/Serialization/Attributes/BsonVectorAttribute.cs

+namespace MongoDB.Bson.Serialization.Attributes
+{
+    /// <summary>
+    /// Sets the representation for this field or property as a BSON Vector with the specified data type.


/// Sets the representation for this field or property as a BSON Vector with the specified data type.

Add as for grammar.

rstam · 2025-02-01T03:58:15Z

src/MongoDB.Bson/ObjectModel/BsonVectorDataType.cs

+        /// <summary>
+        /// The float32
+        /// </summary>
+        Float32 = 0x27,


Doc comments should end in a period.

rstam · 2025-02-01T03:58:22Z

src/MongoDB.Bson/ObjectModel/BsonVectorDataType.cs

+        /// <summary>
+        /// The int8
+        /// </summary>
+        Int8 = 0x03,


Doc comments should end in a period.

rstam · 2025-02-01T04:00:08Z

src/MongoDB.Bson/Serialization/BsonBinaryDataExtensions.cs

+        /// </summary>
+        /// <param name="binaryData">The binary data.</param>
+        /// <returns>Vector data, padding and datatype</returns>
+        public static (TItem[] Items, byte Padding, BsonVectorDataType vectorDataType) ToBsonVectorAsArray<TItem>(this BsonBinaryData binaryData)


Shouldn't vectorDataType start with an upper case V?

rstam · 2025-02-01T04:00:43Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+            return (items, padding, vectorDataType);
+        }
+
+        public static (ReadOnlyMemory<byte> Byte, byte Padding, BsonVectorDataType VectorDataType) ReadBsonVectorAsBytes(ReadOnlyMemory<byte> vectorData)


Shouldn't Byte be `Bytes``? (plural)

rstam · 2025-02-01T04:00:58Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+            switch (vectorDataType)
+            {
+                case BsonVectorDataType.Float32:
+                        return new BsonVectorFloat32(AsTypedArrayOrThrow<float>()) as BsonVectorBase<TItem>;


Fix indentation in these three cases.

rstam · 2025-02-01T04:02:00Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+                _ => throw new ArgumentException(nameof(bsonVectorDataType), "Unsupported vector datatype.")
+            };
+
+            if (expectedItemType != typeof(TItem))


if (typeof(TItem) != expectedItemType)

This makes more sense if it reads more like English: "if the item type is not equal to the expected item type"

rstam · 2025-02-01T04:10:30Z

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

+                _ => throw new ArgumentException(nameof(bsonVectorDataType), "Unsupported vector datatype.")
+            };
+
+            if (supportedType != typeof(TItem))


I agree in with variables on the left and constants on the right.

But two things are different here:

typeof(TItem) is not a constant, it's an expression involving the type variable TItem

we also like to standardize on if (actualXyz != expectedXyz)

Technically typeof(TItem) will become a constant at runtime but ONLY after the generic method is Jitted for a particular TItem. But for us as programmers TItem is a type variable.

CSHARP-5202: BSON Binary Vector Subtype Support

7db8497

BorisDog requested a review from a team as a code owner December 27, 2024 00:28

BorisDog requested review from JamesKovacs, papafe and sanych-sun and removed request for a team and JamesKovacs December 27, 2024 00:28

papafe reviewed Dec 30, 2024

View reviewed changes

BorisDog and others added 2 commits January 2, 2025 14:59

- PR comments

c1dda43

PR comments

9a3ce23

Co-authored-by: Ferdinando Papale <[email protected]>

BorisDog requested a review from papafe January 2, 2025 23:00

sanych-sun requested changes Jan 21, 2025

View reviewed changes

- Support for BsonVector in VectorSearch

19e00f1

- PR comments

BorisDog commented Jan 27, 2025

View reviewed changes

BorisDog requested a review from sanych-sun January 27, 2025 20:01

BorisDog and others added 5 commits January 27, 2025 12:03

Merge branch 'csharp5202' of https://github.com/BorisDog/mongo-csharp…

20c5533

…-driver into csharp5202

CSHARP-5202: BSON Binary Vector Subtype Support

1eb8d01

- PR comments

bef489d

- Support for BsonVector in VectorSearch

bb711d9

- PR comments

PR comments

cd2e772

Co-authored-by: Ferdinando Papale <[email protected]>

sanych-sun requested changes Jan 27, 2025

View reviewed changes

- VectorSearchTests update

1243134

- Rebase

BorisDog force-pushed the csharp5202 branch from 20c5533 to 1243134 Compare January 27, 2025 21:08

- PR comments

13eb26e

BorisDog requested review from rstam and sanych-sun January 28, 2025 22:03

rstam requested changes Jan 29, 2025

View reviewed changes

- Added validation for zero padding in non BsonVector data types

589900b

- PR comments

BorisDog requested a review from rstam January 30, 2025 00:31

rstam requested changes Jan 30, 2025

View reviewed changes

BorisDog added 3 commits January 30, 2025 15:02

- PR comments

96d7445

- RP comments

a7eb949

- PR comments

1c08a6f

BorisDog commented Jan 31, 2025

View reviewed changes

BorisDog requested a review from rstam January 31, 2025 01:06

- PR comments

b4c7383

rstam requested changes Jan 31, 2025

View reviewed changes

- PR comments

453ac64

BorisDog requested a review from rstam February 1, 2025 00:27

rstam requested changes Feb 1, 2025

View reviewed changes

CSHARP-5202: BSON Binary Vector Subtype Support #1581

Are you sure you want to change the base?

CSHARP-5202: BSON Binary Vector Subtype Support #1581

Conversation

BorisDog commented Dec 27, 2024

papafe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstam Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstam Jan 30, 2025 •

edited

Loading