Skip to content

.Net: Add covariant return types to VectorStore.GetCollection() implementations - #11883

Merged
Adam Sitnik (adamsitnik) merged 1 commit into
microsoft:feature-vector-data-preb3from
roji:CovariantGetCollection
May 5, 2025
Merged

Adam Sitnik (adamsitnik) merged 1 commit into
microsoft:feature-vector-data-preb3from
roji:CovariantGetCollection

Conversation

@roji

@roji Shay Rojansky (roji) commented May 4, 2025 •

Copy link
Copy Markdown
Member

** NOTE: This PR is based on top of #11882, review last commit only **

Note that covariant return types are only supported in C# 9.0 and later, so there's #ifs in place for .NET Framework.

Closes #11759

@roji
Shay Rojansky (roji) requested a review from a team as a code owner May 4, 2025 16:15
@markwallace-microsoft Mark Wallace (markwallace-microsoft) added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory labels May 4, 2025
@github-actions github-actions Bot changed the title Add covariant return types to VectorStore.GetCollection() implementations .Net: Add covariant return types to VectorStore.GetCollection() implementations May 4, 2025

@adamsitnik Adam Sitnik (adamsitnik) left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not big fan of pragmas and #if defines, but taking that aside I wonder if this change won't do more harm than good to our customer.

Let's consider following example:

AzureAISearchVectorStore store = new(/* omitted for brevity */);
AzureAISearchCollection<string, MyRecord> collection = store.GetCollection<string, MyRecord>(/* omitted for brevity */);

the code is going to compile for those who target .NET 8+ only, but cause an error for:

  • libraries that also target .NET Standard 2.0.
  • test projects that target Full Framework

I know that most likely such scenarios are going to be rare, but I just want to make sure these were considered. (and yes, is this case the user could just use var, but there are other scenarios like passing to a method)

@roji

Copy link
Copy Markdown
Member Author

Yep, this indeed (slightly) diverges the API for framework/netstandard2.0 vs. net8.0 - and it's slightly less nice for provider implementors. But as much as possible, I think it's important to not compromise the developer experience of modern .NET users just because we also support old .NET users... We don't currently have much provider-specific APIs, but this is something I expect will change in the future, and at that point users of e.g. Azure AI Search should be able to access such APIs without ugly downcasts. Projects that need to still support old .NET can simply not depend on this feature, and write code that expects VectorStoreCollection rather than AzureAISearchCollection - that'll compile and work everywhere.

BTW the designers of System.Data also placed a lot of importance on the single-provider users getting the provider-specific types rather than the abstraction types. Since covariant return types didn't exist back then, they implemented the following pattern:

class DbCommand
{
    // Non-virtual public method. This gets hidden 
    public DbDataReader ExecuteReader() => ExecuteDbDataReader();

    // Virtual protected 
    protected abstract DbDataReader ExecuteDbDataReader();
}

class NpgsqlCommand
{
    // Hide the base non-virtual ExecuteReader. So if a user has an NpgsqlCommand, calling ExecuteReader()
    // on it returns an NpgsqlDataReader (great). But if the user has a DbCommand, calling ExecuteReader()
    // binds to the base method, and returns a DbDataReader. That's a sort of implementation of covariant types.
    public new NpgsqlDataReader ExecuteReader() => ...;

    protected override DbDataReader ExecuteDbDataReader() => ...;
}

We could implement this pattern instead of using .NET covariant return types, but it's a bit convoluted; I'm also not too concerned with optimizing the experience for .NET Framework users (they're already excluded from many good new stuff) - I'm mainly thinking about making sure modern .NET users get the best experience they can.

@adamsitnik Adam Sitnik (adamsitnik) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shay Rojansky (@roji) let's get it merged then!

This branch was previously deployed

1 inactive deployment
integration — 955f05f0 Deployed May 5, 2025 by roji
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernel Issues or pull requests impacting the core kernel memory .NET Issue or Pull requests regarding .NET code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants