feat: Add generic ITextSearch<TRecord> interface with LINQ filtering#1
Closed
alzarei wants to merge 1 commit intofeature/issue-10456-linq-filteringfrom
Closed
Conversation
- Add ITextSearch<TRecord> generic interface with type-safe LINQ filtering - Add TextSearchOptions<TRecord> with Expression<Func<TRecord, bool>>? Filter property - Update VectorStoreTextSearch to implement both generic and legacy interfaces - Maintain full backward compatibility with existing ITextSearch interface - Enable compile-time type safety and eliminate TextSearchFilter conversion overhead Addresses microsoft#10456
1120208 to
3050216
Compare
alzarei
added a commit
that referenced
this pull request
Feb 12, 2026
…r AOT compatibility Fixes critical AOT incompatibility in BraveTextSearch and TavilyTextSearch by removing Expression.Lambda().Compile().DynamicInvoke() calls that break NativeAOT compilation. The fallback case in ExtractValue() now throws NotSupportedException with a clear message explaining that only constant expressions and simple member access are supported for AOT compatibility. This resolves Copilot feedback Issue #1 on PR microsoft#13384.
alzarei
added a commit
that referenced
this pull request
Feb 13, 2026
…rave/Tavily TextSearch (microsoft#13541) # PR: Fix AOT Compatibility - Remove Expression.Compile() from Brave/Tavily TextSearch ## Motivation and Context This PR addresses **Critical Issue #1** from Copilot's code review feedback on PR microsoft#13384 (feature-text-search-linq → main). **Problem:** The `ExtractValue()` method in both `BraveTextSearch` and `TavilyTextSearch` uses `Expression.Lambda(expression).Compile().DynamicInvoke()` as a fallback case, which breaks NativeAOT compatibility. This directly contradicts the ADR-0065 architectural decision that states: > "Both interfaces are AOT-compatible with no `[RequiresDynamicCode]` attributes" **Impact:** - Breaks NativeAOT compilation scenarios - Violates Semantic Kernel's AOT compatibility requirements - Contradicts documented architectural decisions - Blocks deployment scenarios requiring AOT **Root Cause:** The issue was introduced in PR microsoft#13191 (Tavily/Brave connector modernization) without AOT consideration. ## Description Replaces the AOT-incompatible `Expression.Lambda(expression).Compile().DynamicInvoke()` fallback with a `NotSupportedException` that: - Maintains AOT compatibility (no dynamic code generation) - Provides clear error message explaining supported expression types - Includes expression type and value for debugging - Guides users to use only constant expressions and simple member access ## Changes Made ### Files Modified - `dotnet/src/Plugins/Plugins.Web/Brave/BraveTextSearch.cs` - `dotnet/src/Plugins/Plugins.Web/Tavily/TavilyTextSearch.cs` ### Before (AOT-incompatible): ```csharp private static object? ExtractValue(Expression expression) { return expression switch { ConstantExpression constant => constant.Value, MemberExpression member when member.Expression is ConstantExpression constantExpr => member.Member switch { System.Reflection.FieldInfo field => field.GetValue(constantExpr.Value), System.Reflection.PropertyInfo property => property.GetValue(constantExpr.Value), _ => null }, _ => Expression.Lambda(expression).Compile().DynamicInvoke() // ❌ BREAKS AOT }; } ``` ### After (AOT-compatible): ```csharp private static object? ExtractValue(Expression expression) { return expression switch { ConstantExpression constant => constant.Value, MemberExpression member when member.Expression is ConstantExpression constantExpr => member.Member switch { System.Reflection.FieldInfo field => field.GetValue(constantExpr.Value), System.Reflection.PropertyInfo property => property.GetValue(constantExpr.Value), _ => null }, _ => throw new NotSupportedException( $"Unable to extract value from expression of type '{expression.GetType().Name}'. " + $"Only constant expressions and simple member access are supported for AOT compatibility. " + $"Expression: {expression}") }; } ``` ## Supported Expression Patterns ✅ **Works (AOT-compatible):** - Constant values: `page.Language == "en"` - Simple variables: `var lang = "en"; page.Language == lang` - Member access: `config.Language` (where `config` is a captured variable) ❌ **Not Supported (would require dynamic compilation):** - Computed values: `page.Language == someVariable` - Method calls: `page.Language == GetLanguage()` - Complex expressions: `page.Language == (isEnglish ? "en" : "fr")` Users encountering the exception should extract the value to a variable before using it in the filter expression. ## Testing - [x] Verified changes compile successfully - [x] Confirmed AOT compatibility (no `Expression.Compile()` usage) - [x] Exception message provides clear guidance - [ ] Unit tests pass (pending CI/CD validation) ## Related Issues/PRs - **Parent PR:** microsoft#13384 (feature-text-search-linq → main) - **Copilot Review:** Issue #1 - AOT Compatibility (Critical) - **Root Cause:** PR microsoft#13191 (Tavily/Brave modernization) - **ADR Reference:** docs/decisions/0065-linq-based-text-search-filtering.md (PR microsoft#13335) ## Contribution Checklist - [ ] The code builds clean without any errors or warnings - [x] Appropriate tests have been added (existing tests remain) - [x] Changes are documented as needed - [x] AOT compatibility verified ## Reviewer Notes @markwallace-microsoft, @westey-m This is the first of two critical fixes for PR microsoft#13384. The fix: 1. Eliminates all `Expression.Compile()` usage for AOT compatibility 2. Provides clear error messages for unsupported expression patterns 3. Aligns implementation with ADR-0065's AOT compatibility guarantee The exception should rarely be hit in practice since most filters use simple constants or variables (which are handled by the first two cases). Users with complex expressions will get immediate, clear feedback on how to fix their code. **Next Steps:** - Issue #2 (Breaking Change - Legacy Result Type) requires design decision - Issues #3-4 (Moderate bugs) can be addressed in follow-up PR --------- Co-authored-by: Alexander Zarei <alzarei@users.noreply.github.com>
alzarei
added a commit
that referenced
this pull request
Feb 19, 2026
microsoft#13564) # .Net: fix: Remove OldFilter dependency from VectorStoreTextSearch (microsoft#10456) **Addresses @roji's [feedback](microsoft#13384 (comment)) on PR microsoft#13384**: Remove the dependency on MEVD's obsolete `OldFilter` property from `VectorStoreTextSearch`, enabling MEVD to drop `OldFilter` before publishing 1.0 provider versions. > **Context** > This is a follow-up change on the `feature-text-search-linq` branch addressing @roji's review comment. The change converts the legacy `ITextSearch` code path from using `VectorSearchFilter.OldFilter` to building a LINQ expression tree via `BuildFilterExpression()`, so both legacy and modern paths now converge on `VectorSearchOptions<TRecord>.Filter`. ### Motivation and Context <!-- Thank you for your contribution to the semantic-kernel repo! Please help reviewers and future users, providing the following information: 1. Why is this change required? 2. What problem does it solve? 3. What scenario does it contribute to? 4. If it fixes an open issue, please link to the issue here. --> **Why is this change required?** @roji [identified](microsoft#13384 (comment)) that `VectorStoreTextSearch`'s legacy path hands `FilterClause` values directly to MEVD via `OldFilter`, which means MEVD **cannot** remove `OldFilter` while that dependency exists. His priority is to remove `OldFilter` from MEVD before publishing 1.0 versions of most providers — an `[Obsolete]` property on a 1.0 API is undesirable. **What problem does it solve?** - Removes the last SK-level dependency on MEVD's obsolete `OldFilter` property - Enables MEVD to drop `OldFilter` independently, unblocking 1.0 provider releases - Eliminates `#pragma warning disable CS0618` suppressions for `OldFilter`/`VectorSearchFilter` usage - Corrects the ADR-0065 Option 2 rejection rationale (building expression trees ≠ compiling them) **What scenario does it contribute to?** Unblocks MEVD 1.0 provider releases by removing the coupling between SK's legacy text search path and MEVD's obsolete filtering API. **Issue Link:** microsoft#10456 ### Description #### Code Change: `VectorStoreTextSearch.cs` Replaced the legacy path's `OldFilter` assignment with a new `BuildFilterExpression()` method that converts `FilterClause` values to a LINQ expression tree: **Before:** ```csharp #pragma warning disable CS0618 // VectorSearchFilter is obsolete OldFilter = searchOptions.Filter?.FilterClauses is not null ? new VectorSearchFilter(searchOptions.Filter.FilterClauses) : null, #pragma warning restore CS0618 ``` **After:** ```csharp Filter = searchOptions.Filter?.FilterClauses is not null ? BuildFilterExpression(searchOptions.Filter.FilterClauses) : null, ``` **`BuildFilterExpression()` details:** - Iterates `FilterClause` instances, currently handling `EqualToFilterClause` (the only type `TextSearchFilter` produces) - For each clause: resolves `PropertyInfo` via `typeof(TRecord).GetProperty()` (trim-safe because `TRecord` has `[DynamicallyAccessedMembers(PublicProperties)]`), builds `Expression.Property` → `Expression.Equal` → `Expression.AndAlso` - Uses `PropertyInfo` overload of `Expression.Property()` to avoid `IL2026` trimming warning (the `string` overload has `[RequiresUnreferencedCode]`) - Returns `Expression<Func<TRecord, bool>>` or `null` if no clauses - Throws `NotSupportedException` for unsupported clause types, `InvalidOperationException` for missing properties **AOT compatibility:** Building expression trees (`Expression.Property`, `Expression.Equal`, `Expression.Lambda`) is pure data-structure construction — fully AOT-compatible. MEVD's `VectorSearchOptions<TRecord>.Filter` analyzes the tree without calling `Expression.Compile()`. #### ADR Update: `0065-linq-based-text-search-filtering.md` - **Architecture diagram**: Updated Pattern A to show `BuildFilterExpression()` converting clauses to LINQ trees, both paths converging on `VectorSearchOptions.Filter` - **Pattern A description**: Updated to reflect both paths now use `VectorSearchOptions.Filter` - **Option 2 rationale**: Corrected the rejection — original assessment incorrectly claimed `RequiresDynamicCode` would cascade; in fact, only `Expression.Compile()` requires dynamic code, not tree construction. Added update note explaining the distinction. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 ### Validation | Check | Result | |---|---| | Format (`dotnet format --verify-no-changes`) | ✅ Pass | | Build (`dotnet build --configuration Release --warnaserror`) | ✅ Pass (0 warnings, 0 errors) | | Unit tests (`SemanticKernel.UnitTests`) | ✅ Pass (1606/1606) | | AOT publish (`dotnet publish --configuration Release`, net9.0) | ✅ Pass (0 warnings) | | AOT publish (`dotnet publish --configuration Release`, net10.0) | ✅ Pass (0 warnings) | Co-authored-by: Alexander Zarei <alzarei@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add generic ITextSearch interface with LINQ filtering support
Addresses Issue microsoft#10456: Modernize ITextSearch to use LINQ-based vector search filtering
Motivation and Context
Why is this change required?
The current ITextSearch interface uses legacy
TextSearchFilterwhich requires conversion to obsoleteVectorSearchFilter, creating technical debt and performance overhead. Issue microsoft#10456 requests modernization to use type-safe LINQ filtering withExpression<Func<TRecord, bool>>.What problem does it solve?
What scenario does it contribute to?
This enables developers to write type-safe text search filters like:
Issue Link: microsoft#10456
Description
This PR introduces foundational generic interfaces to enable LINQ-based filtering for text search operations. The implementation follows an additive approach, maintaining 100% backward compatibility while providing a modern, type-safe alternative.
Overall Approach:
ITextSearch<TRecord>interface alongside existing non-generic versionTextSearchOptions<TRecord>with LINQExpression<Func<TRecord, bool>>? FilterVectorStoreTextSearchto implement both interfacesUnderlying Design:
Summary
This PR introduces the foundational generic interfaces needed to modernize text search functionality from legacy
TextSearchFilterto type-safe LINQExpression<Func<TRecord, bool>>filtering. This is the first in a series of PRs to completely resolve Issue microsoft#10456.Key Changes
New Generic Interfaces
ITextSearch<TRecord>: Generic interface with type-safe LINQ filteringSearchAsync<TRecord>(string query, TextSearchOptions<TRecord> options, CancellationToken cancellationToken)GetTextSearchResultsAsync<TRecord>(string query, TextSearchOptions<TRecord> options, CancellationToken cancellationToken)GetSearchResultsAsync<TRecord>(string query, TextSearchOptions<TRecord> options, CancellationToken cancellationToken)TextSearchOptions<TRecord>: Generic options class with LINQ supportExpression<Func<TRecord, bool>>? Filterproperty for compile-time type safetyEnhanced Implementation
VectorStoreTextSearch<TValue>: Now implements both generic and legacy interfacesITextSearchITextSearch<TValue>with direct LINQ filteringTextSearchFilter→ obsoleteVectorSearchFilterconversionBenefits
Type Safety & Developer Experience
Performance Improvements
Zero Breaking Changes
ITextSearchandTextSearchOptionsuntouchedImplementation Strategy
This PR implements Phase 1 of the Issue microsoft#10456 resolution across 6 structured PRs:
[DONE] PR 1 (This PR): Core generic interface additions
ITextSearch<TRecord>andTextSearchOptions<TRecord>interfacesVectorStoreTextSearchto implement both legacy and generic interfaces[TODO] PR 2: VectorStoreTextSearch internal modernization
VectorSearchFilterconversion overhead[TODO] PR 3: Modernize BingTextSearch connector
BingTextSearch.csto implementITextSearch<TRecord>[TODO] PR 4: Modernize GoogleTextSearch connector
GoogleTextSearch.csto implementITextSearch<TRecord>[TODO] PR 5: Modernize remaining connectors
TavilyTextSearch.csandBraveTextSearch.cs[TODO] PR 6: Tests and samples modernization
Verification Results
Microsoft Official Pre-Commit Compliance
Test Coverage
Code Quality
Example Usage
Before (Legacy)
After (Generic with LINQ)
Files Modified
Contribution Checklist
Verification Evidence:
dotnet build --configuration Release- 0 warnings, 0 errorsdotnet test --configuration Release- 1,574/1,574 tests passed (100%)dotnet format SK-dotnet.slnx --verify-no-changes- 0/10,131 files needed formattingIssue: microsoft#10456
Type: Enhancement (Feature Addition)
Breaking Changes: None
Documentation: Updated with comprehensive XML docs and usage examples