[MEVD] Make VectorStoreRecordDefinition mutable

Being init-only and readonly makes it difficult to define a collection programmatically.
Suggestion: provide a more flexible way to define collections dynamically. For instance allow “set” and allow mutable lists. I can’t see a scenario where mutability would be a problem.

westey (@westey-m)

"to avoid someone accidentally modifying it after passing it to a collection” => does it really happen? Sounds like premature optimization.

I agree with westey (@westey-m) here - yes, this does really happen (I've seen it in other APIs). Users would create a collection, then reuse the same definition, tweak it a bit and create a 2nd collection, with the unexpected result that the first collection suddenly changed. Immutability here seems to be just good, sensible API designto me.

Devis Lucato (@dluc) can you please give a quick code sample of where you believe this to be making your code more complex?

Also, note that we do support cloning of properties with modifications using the following syntax:

var clone = new VectorStoreRecordDataProperty(originalProp) { IsFilterable = true }

I believe there are other options to lock the state of a definition, e.g. the .NET pattern used by a writable ServiceCollection generating a readonly ServiceProvider, or manually setting a readonly property used in some JSON serialization scenarios.

Note, this is mostly about dev time, when one really can benefit from mutable objects for quicker tests. The current syntax might be beneficial against bugs, but affects dev velocity, so it depends from which angle you're looking at it.

Here's an example, current code vs preferred code.

Current:

VectorStoreRecordDefinition PrepareRecordDefinition(IVectorStore vectorStore, List<FieldDefinition> fields)
{
    var properties = new List<VectorStoreRecordProperty>(); // <===
    foreach (var f in fields)
    {
        switch (f.Type)
        {
            case FieldTypes.PrimaryKey:
                properties.Add(vectorStore is QdrantVectorStore
                    ? new VectorStoreRecordKeyProperty(f.Name, typeof(Guid))
                    : new VectorStoreRecordKeyProperty(f.Name, typeof(string)));

                break;
            // ...
            case FieldTypes.Text:
                properties.Add(new VectorStoreRecordDataProperty(f.Name, typeof(string))
                {
                    IsFilterable = f.IsFilterable,
                    IsFullTextSearchable = f.IsFullTextSearchable
                });
                break;
            // ...
        }
    }

    return new VectorStoreRecordDefinition { Properties = properties }; // <===
}

Preferred:

VectorStoreRecordDefinition PrepareRecordDefinition(IVectorStore vectorStore, List<FieldDefinition> fields)
{
    var result = new VectorStoreRecordDefinition(); // <===
    foreach (var f in fields)
    {
        switch (f.Type)
        {
            case FieldTypes.PrimaryKey:
                result.Properties.Add(vectorStore is QdrantVectorStore
                    ? new VectorStoreRecordKeyProperty(f.Name, typeof(Guid))
                    : new VectorStoreRecordKeyProperty(f.Name, typeof(string)));

                break;
            // ...
            case FieldTypes.Text:
                result.Properties.Add(new VectorStoreRecordDataProperty(f.Name, typeof(string))
                {
                    IsFilterable = f.IsFilterable,
                    IsFullTextSearchable = f.IsFullTextSearchable
                });
                break;
            // ...
        }
    }

    return result; // <===
}

There are other issues (e.g. checking vectorStore concrete class), but here the focus is on the local properties var.

I believe there are other options to lock the state of a definition, e.g. the .NET pattern used by a writable ServiceCollection generating a readonly ServiceProvider, or manually setting a readonly property used in some JSON serialization scenarios.

Those patterns make sense for big, configurable and extensible things - ServiceCollection can be progressively configured for different things via different extension methods. In contrast, here we're talking about a tiny type with 3-4 properties; requiring users to go through the builder pattern for each an every VectorStoreRecordProperty that the user needs to configure would make the code extremely verbose for no good reason, IMHO.

If anything, .NET has moved more in the direction of immutable types and "non-destructive mutation" (i.e. create a copy) - records and the C# with syntax are a good example of this.

Note, this is mostly about dev time, when one really can benefit from mutable objects for quicker tests. The current syntax might be beneficial against bugs, but affects dev velocity, so it depends from which angle you're looking at it.

OK, but looking at your code, that extra instantiation of a List<VectorStoreRecordProperty> seems to be far, far less important then e.g. a user accidentally modifying a definition that's already in use by some active collection, causing concurrency issues etc. Immutability is simply good basic API design here, IMHO.

I'll also point out that in the majority of cases, users just have a static list of properties to configure, and that scenario works quite well with the current API shape:

var definition = new VectorStoreRecordDefinition
{
    Properties = [...]
};

So the extra list would only be needed in various dynamic scenarios, which are quite a bit more rare/advanced.

Make record definition property types mutable

.Net: Pack of changes from MEVD API review (#11882)

Since we had the above discussion, we made changes so that the record definition isn't actually referenced and used from MEVD - it's only used as input for constructing the internal MEVD model (along with the attributes). That means that it's OK to mutate it once collection construction is complete.

We also had an API review on the option bag types (e.g. RecordRetrievalOptions) where it was decided to make these mutable; I went ahead and did the same here.

Done in #11882

Navigation Menu

Repository navigation

[MEVD] Make VectorStoreRecordDefinition mutable #10879

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[MEVD] Make VectorStoreRecordDefinition mutable #10879

Description

Activity

roji commented on Mar 11, 2025

westey-m commented on Mar 11, 2025

dluc commented on Mar 12, 2025

roji commented on Mar 13, 2025

roji commented on May 5, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions