Skip to content

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

Merged
adamsitnik merged 1 commit intomicrosoft:feature-vector-data-preb3from
roji:CovariantGetCollection
May 5, 2025
Merged

.Net: Add covariant return types to VectorStore.GetCollection() implementations#11883
adamsitnik merged 1 commit intomicrosoft:feature-vector-data-preb3from
roji:CovariantGetCollection

Conversation

@roji
Copy link
Member

@roji roji commented May 4, 2025

** 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 roji requested a review from a team as a code owner May 4, 2025 16:15
@markwallace-microsoft 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
@roji roji force-pushed the CovariantGetCollection branch from 4360b38 to d60fc90 Compare May 4, 2025 16:23
@roji roji temporarily deployed to integration May 4, 2025 16:24 — with GitHub Actions Inactive
@roji roji force-pushed the CovariantGetCollection branch from d60fc90 to fe6f603 Compare May 4, 2025 22:36
@roji roji temporarily deployed to integration May 4, 2025 22:37 — with GitHub Actions Inactive
Copy link
Member

@adamsitnik adamsitnik left a comment

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
Member Author

roji commented May 5, 2025

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.

@roji roji force-pushed the CovariantGetCollection branch from fe6f603 to 955f05f Compare May 5, 2025 14:50
@roji roji temporarily deployed to integration May 5, 2025 14:50 — with GitHub Actions Inactive
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@roji let's get it merged then!

@adamsitnik adamsitnik merged commit 280d98b into microsoft:feature-vector-data-preb3 May 5, 2025
11 checks passed
@roji roji deleted the CovariantGetCollection branch May 5, 2025 17:28
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