Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.Net: Bug: .NET search option classes duplication #11270

Open
dluc opened this issue Mar 28, 2025 · 5 comments
Open

.Net: Bug: .NET search option classes duplication #11270

dluc opened this issue Mar 28, 2025 · 5 comments
Assignees
Labels
bug Something isn't working msft.ext.vectordata Related to Microsoft.Extensions.VectorData .NET Issue or Pull requests regarding .NET code

Comments

@dluc
Copy link
Contributor

dluc commented Mar 28, 2025

This is probably already in your radar but I don't see it in the backlog.

These two classes are very similar and could be consolidated to avoid code duplication.

  • Microsoft.Extensions.VectorData.VectorSearchOptions<TRecord>
  • Microsoft.Extensions.VectorData.HybridSearchOptions<TRecord>

Note: Top and Skip validation is different

@dluc dluc added bug Something isn't working msft.ext.vectordata Related to Microsoft.Extensions.VectorData labels Mar 28, 2025
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code triage labels Mar 28, 2025
@github-actions github-actions bot changed the title Bug: .NET search option classes duplication .Net: Bug: .NET search option classes duplication Mar 28, 2025
@roji
Copy link
Member

roji commented Mar 31, 2025

HybridSearchOptions has AdditionalProperty to select the full-text property - IIRC that's the reason for having two (/cc @westey-m). We could make HybridSearchOptions extend VectorSearchOptions if we want.

@westey-m
Copy link
Contributor

Yep @roji, that's correct. HybridSearchOptions does have AdditionalProperty.
That said, since everything on both options classes are optional, including AdditionalProperty, we could introduce some simple lossy helpers to convert between these types in case a developer has some basic options they just want to use everywhere.

@roji
Copy link
Member

roji commented Mar 31, 2025

Sure. Though just having two global/singleton instances (for each type) instead of just one doesn't seem too bad. I personally don't think there's anything urgent to do here, unless we maybe want to have them in a hierarchy...

@westey-m
Copy link
Contributor

Unless I missed something, even introducing a hierarchy shouldn't be a breaking change, so agreed, not urgent.
I'd also hold off on the hierarchy until we have finished all our search variations, to avoid creating a bad hierarchy that needs to be broken again.

@markwallace-microsoft
Copy link
Member

HybridSearchOptions doesn't validate top and skip but VectorSearchOptions does. Keeping this issue to resolve that inconsistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working msft.ext.vectordata Related to Microsoft.Extensions.VectorData .NET Issue or Pull requests regarding .NET code
Projects
Status: Bug
Development

No branches or pull requests

4 participants