.Net: Shorten MEVD implementation type names #11879
Conversation
markwallace-microsoft
added
.NET
github-actions
bot
changed the title
There was a problem hiding this comment.
LGTM, I love the simplicity of the new names! Thank you @roji !
| /// <returns>The record key.</returns> | ||
| public delegate TKey? InMemoryVectorStoreKeyResolver<TKey, TRecord>(TRecord record) | ||
| [Experimental("MEVD9000")] | ||
| public delegate TKey? InMemoryKeyResolver<TKey, TRecord>(TRecord record) |
There was a problem hiding this comment.
We should try to replace a custom delgate with Func if possible (I see it has a generic constraint, but if it's being used with a method that already has such constraint, we could get rid of it)
I don't ask for doing this in this PR, we could just give it a try and maybe avoid the need of having another experimental type
There was a problem hiding this comment.
I've sent #11889 to address this
There was a problem hiding this comment.
I'm not quite sure whether these resolvers are necessary or what the motivation for them is - we may just want to remove them altogether... I'll bring it up in an upcoming standup. In any case, they're flagged as experimental so we always have the option to remove them later.
| /// <param name="record">The record that contains the vector to look up.</param> | ||
| /// <returns>The named vector from the record.</returns> | ||
| public delegate object? InMemoryVectorStoreVectorResolver<TRecord>(string vectorName, TRecord record); | ||
| [Experimental("MEVD9000")] |
There was a problem hiding this comment.
Same as for the other reslolver delegate: could we make it a Func?
| namespace Microsoft.SemanticKernel.Connectors.SqlServer; | ||
|
|
||
| internal sealed class RecordMapper<TRecord>(CollectionModel model) | ||
| internal sealed class SqlServerMapper<TRecord>(CollectionModel model) |
There was a problem hiding this comment.
FWIW I have used the RecordMapper name on purpose: it's an internal type of the Sql package, so for me there was no need to use the Sql prefix.
There was a problem hiding this comment.
Right, the only thing is to make it easier for us to navigate the codebase in the IDE, i.e. to jump directly to the mapper of a specific provider... Otherwise, if we have lots of RecordMappers in different providers, navigations becomes difficult...
Now with the shorter names, PostgresMapper, SqlServerMapper etc. seem reasonable enough to me in any case...
adamsitnik
merged commit 1d82e6c
into
microsoft:feature-vector-data-preb3
3 participants
Following #11832, this shortens the names of provider implementation types as discussed in API review (e.g. AzureCosmosDBMongoDBVectorStoreRecordCollection -> CosmosMongoCollection)
Closes #11230