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

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.

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>


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.

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.

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.

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

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)


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

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)


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

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.

Right it's always confusing. It's needs to be private 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}"/>

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

src/MongoDB.Bson/ObjectModel/BsonVectorDataType.cs

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?

It should, done.

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.

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

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.

rstam

Added new comments related to changes required to allow users to configure serialization in code rather than using attributes.

rstam · 2025-02-03T16:21:32Z

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

+    /// </summary>
+    /// <typeparam name="TItemContainer">The items container, for example <see cref="BsonVectorBase{TItem}"/> or <see cref="Memory{TItem}"/>.</typeparam>
+    /// <typeparam name="TItem">The .NET data type.</typeparam>
+    internal abstract class BsonVectorSerializerBase<TItemContainer, TItem> : SerializerBase<TItemContainer>


This class and all of its subclasses need to be public so that users can configure class maps in code.

Some users don't want to (or can't) put attributes on their model classes and either prefer or are forced to configure their serialization separately in code.

rstam · 2025-02-03T16:22:48Z

tests/MongoDB.Bson.Tests/Serialization/Attributes/BsonVectorAttributeTests.cs

+
+            [BsonVector(BsonVectorDataType.Float32)]
+            public float[] ValuesFloat { get; set; }
+        }


After making the serializers for values represented as vectors public the same configuration can be achieved in code as follows:

var cm = new BsonClassMap<ArrayHolder>(); cm.AutoMap(); cm.MapMember(x => x.ValuesByte).SetSerializer(new ArrayAsBsonVectorSerializer<byte>(BsonVectorDataType.Int8)); cm.MapMember(x => x.ValuesPackedBit).SetSerializer(new ArrayAsBsonVectorSerializer<byte>(BsonVectorDataType.PackedBit)); cm.MapMember(x => x.ValuesFloat).SetSerializer(new ArrayAsBsonVectorSerializer<float>(BsonVectorDataType.Float32));

But... we can make the user's life easier by defining the following three subclasses:

public class ByteArrayAsInt8VectorSerializer() : ArrayAsBsonVectorSerializer<byte>(BsonVectorDataType.Int8); public class ByteArrayAsPackedBitVectorSerializer() : ArrayAsBsonVectorSerializer<byte>(BsonVectorDataType.PackedBit); public class SingleArrayAsFloat32VectorSerializer() : ArrayAsBsonVectorSerializer<byte>(BsonVectorDataType.Float32);

which allow the user to configure the serializers without using the more complicated constructor call requiring a generic type parameter and a vector data type:

cm.MapMember(x => x.ValuesByte).SetSerializer(new ByteArrayAsInt8VectorSerializer()); cm.MapMember(x => x.ValuesPackedBit).SetSerializer(new ByteArrayAsPackedBitVectorSerializer()); cm.MapMember(x => x.ValuesFloat).SetSerializer(new SingleArrayAsFloat32VectorSerializer());

Besides being more compact, it is less error prone (less likely to use invalid generic type parameters or mis-matched vector data types).

Similarly for the Memory and ReadOnlyMemory serializers.

I think it's potentially a good suggestion.
But I'd like to consider it later, hopefully after collecting some user feedback.

BTW we already have some future potential additions to BSON Vector functionality, like BigEndian, other primitive types which are not part of this PR.

OK. This is probably the ONLY thing that can't be configured in code now.

Keep in mind that some users don't want to pollute their model classes with MongoDB attributes. Or in some cases they can't because they don't control the code. For those users configuring serialization in code is important.

Attributes are also worthless if you want to configure the same class to be configured two different ways depending on where they are used. Admittedly a very rare use case.

I may have misinterreted "I'd like to consider it later".

I thought you meant make the serializers public later. But looks like you did make them public.

Yes, I just meant considering the shortcuts later.
Maybe achieving the same by making static BsonVectorSerializer public and extending it.

rstam · 2025-02-03T21:51:54Z

tests/MongoDB.Bson.Tests/Serialization/Attributes/BsonVectorAttributeTests.cs

+
+            [BsonVector(BsonVectorDataType.Float32)]
+            public float[] ValuesFloat { get; set; }
+        }


OK. This is probably the ONLY thing that can't be configured in code now.

Keep in mind that some users don't want to pollute their model classes with MongoDB attributes. Or in some cases they can't because they don't control the code. For those users configuring serialization in code is important.

Attributes are also worthless if you want to configure the same class to be configured two different ways depending on where they are used. Admittedly a very rare use case.

rstam · 2025-02-03T22:01:58Z

src/MongoDB.Bson/ObjectModel/BsonVector.cs

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


This class should really be called BsonVector.

I realize there is a pseudo-conflict with [BsonVector] attribute and that's why you chose this unappealing name.

If we can't rename the attribute, then let's just live with the pseudo-conflict. It's not really a conflict because the attribute class name is actually BsonVectorAttribute.

rstam · 2025-02-03T22:02:28Z

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

+    /// Represents a serializer for <see cref="ReadOnlyMemory{TItem}"/> represented as a BsonVector.
+    /// </summary>
+    /// <typeparam name="TItem">The .NET data type.</typeparam>
+    internal sealed class ReadOnlyMemoryAsBsonVectorSerializer<TItem> : ItemContainerAsBsonVectorSerializer<ReadOnlyMemory<TItem>, TItem>


You probably meant for this one to be public too.

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

src/MongoDB.Bson/Serialization/BsonVectorReader.cs

rstam · 2025-02-03T22:07:22Z

tests/MongoDB.Bson.Tests/Serialization/Attributes/BsonVectorAttributeTests.cs

+
+            [BsonVector(BsonVectorDataType.Float32)]
+            public float[] ValuesFloat { get; set; }
+        }


I may have misinterreted "I'd like to consider it later".

I thought you meant make the serializers public later. But looks like you did make them public.

rstam

LGTM with reluctance due to the names BsonVectorBase and BsonValueAttribute.

The implementation is solid though. It's only the names which give me pause.

sanych-sun · 2025-02-04T01:19:41Z

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

+    /// Sets the representation for this field or property as a BSON Vector with the specified data type.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field)]
+    public sealed class BsonVectorAttribute : Attribute, IBsonMemberMapAttribute


I have an alternative idea: what if we do not introduce BsonVectorAttribute at all, but instead instruct users to specify required serializer via BsonSerializerAttribute. The main motivation for this suggestion is the fact that all other attributes we have are really adjusting the mapped serializer, when in this case we really want to use DIFFERENT serializer then default for this Clr data type. And if we want different - then let's say that aloud =).

Let's discuss it offline may be.

Discussed offline.
BsonSerializerAttribute could also work, but it will require awkward configuration of the additional parameter (and adding support of serializer parameters).
Also it is not as straight forward as BsonVector.

I don't think you are consistent in your arguments. You keep saying that:

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

is SHORT and STRAIGHTFORWARD and yet you reject even more compact and straightforward suggestions like:

[AsInt8Vector] public byte[] A;

I think I am :)
IMO having multiple attributes instead of a single simple BsonVector is less straightforward. Try imaging the user experience of having just a single entry point of BsonVector, where the rest of the params are self explanatory, versus the need of traversing our docs each time to find the right attribute for the right type.

Also I don't think the AsSomething naming pattern for attributes is ideal for us. This also collides with the potential idea of having BsonDoubleAttribute and similar in the future.

The flip side of your BsonDoubleAttribute argument is that we should be able to use BsonVector instead of BsonVectorBase as the type name for the BSON vector class.

If you plan on having BsonDouble and [BsonDouble] coexist there is no reason that BsonVector and [BsonVector] can't coexist.

I think the AsSomething pattern is a good one and one we should have adopted from the very beginning. If we are looking to change how our attributes are defined in the future this would be a good direction to move in.

[AsDouble] public int X;

is very simple and straightforward. It reads very well: X is an int that is represented AS a double in the database.

sanych-sun

LGTM

papafe

A small note, but otherwise LGTM

papafe · 2025-02-05T10:02:04Z

src/MongoDB.Bson/Serialization/BsonVectorExtensions.cs

+        /// <summary>
+        /// Converts <see cref="BsonVectorBase{TItem}"/> to <see cref="BsonBinaryData"/>.
+        /// </summary>
+        /// <typeparam name="TItem"></typeparam>


TItem is missing

Thanks. Done.

papafe

A small note, but otherwise LGTM

rstam

Looks like all the renaming from BsonVector to BinaryVector was done consistently.

To me the last remaining question is whether the <Type>Attribute naming pattern is viable.

I've started a thread in #csharp-driver-devs to discuss as a team before committing to this public API.

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 requested a review from papafe January 2, 2025 23:00

sanych-sun requested changes Jan 21, 2025

View reviewed changes

BorisDog commented Jan 27, 2025

View reviewed changes

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

sanych-sun requested changes Jan 27, 2025

View reviewed changes

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

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

rstam requested changes Jan 29, 2025

View reviewed changes

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

rstam requested changes Jan 30, 2025

View reviewed changes

BorisDog commented Jan 31, 2025

View reviewed changes

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

rstam requested changes Jan 31, 2025

View reviewed changes

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

rstam requested changes Feb 1, 2025

View reviewed changes

rstam requested changes Feb 3, 2025

View reviewed changes

BorisDog requested a review from rstam February 3, 2025 19:15

rstam requested changes Feb 3, 2025

View reviewed changes

BorisDog requested a review from rstam February 3, 2025 22:44

rstam approved these changes Feb 3, 2025

View reviewed changes

sanych-sun reviewed Feb 4, 2025

View reviewed changes

CSHARP-5202: BSON Binary Vector Subtype Support

166824a

BorisDog force-pushed the csharp5202 branch from c3340dc to 166824a Compare February 4, 2025 23:32

BorisDog requested a review from sanych-sun February 5, 2025 00:42

sanych-sun approved these changes Feb 5, 2025

View reviewed changes

papafe approved these changes Feb 5, 2025

View reviewed changes

- BsonVector to BinaryVector change.

4529d60

rstam self-requested a review February 5, 2025 21:45

rstam reviewed Feb 5, 2025

View reviewed changes

rstam self-requested a review February 5, 2025 22:46

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

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