.Net: Add covariant return types to VectorStore.GetCollection() implementations#11883
Conversation
4360b38 to
d60fc90
Compare
d60fc90 to
fe6f603
Compare
There was a problem hiding this comment.
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)
|
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. |
fe6f603 to
955f05f
Compare
adamsitnik
left a comment
There was a problem hiding this comment.
@roji let's get it merged then!
280d98b
into
microsoft:feature-vector-data-preb3
** 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