.Net: Net: Address roji/westey review feedback round 2 on text search connectors (#10456) - #13611
Conversation
- Add [Obsolete] to FilterClause, EqualToFilterClause, AnyTagEqualToFilterClause - Add [Obsolete] to non-generic TextSearchOptions and TextSearchFilter - Refactor Google connector: merge 8 methods into 3 using pattern matching - Refactor Bing/Brave/Tavily: pattern matching, remove MemoryExtensions - Add explicit error for collection Contains (Enumerable.Contains) in Google/Bing - Handle null-guard patterns (x != null) in Google expression processing - Remove dead code (MapGoogleFilterToProperty) - Consolidate pragma CS0618 to file-level in all affected files - ExtractFiltersFromLegacy: throw on unsupported FilterClause types
…ptions/TextSearchFilter
7d5f751 to
45e852b
Compare
| @@ -1,5 +1,7 @@ | |||
| // Copyright (c) Microsoft. All rights reserved. | |||
|
|
|||
| #pragma warning disable CS0618 // Obsolete ITextSearch, TextSearchOptions, TextSearchFilter, FilterClause - backward compatibility | |||
There was a problem hiding this comment.
Ideally these would still be around the code using the obsolete legacy filtering stuff rather than over the entire file... Like I'd group all the legacy APIs in a #region and apply the suppression there.
Or is it too much somehow?
There was a problem hiding this comment.
Thanks! Yes, I like the suggested style too. Will revise shortly.
There was a problem hiding this comment.
Done
There was a problem hiding this comment.
Alexander Zarei (@alzarei) I'm still seeing a file-scoped suppression here - the point is to more narrowly scope the suppression so we don't accidentally use another unrelated obsolete API etc.
| /// A <see cref="FilterClause"/> is used to request that the underlying search service should | ||
| /// filter search results based on the specified criteria. | ||
| /// </remarks> | ||
| [Obsolete("Use LINQ expressions via TextSearchOptions<TRecord>.Filter instead. This type will be removed in a future version.")] |
There was a problem hiding this comment.
The obsoletion message is incorrect - we're in MEVD, so we shouldn't be mentioning TextSearchOptions
There was a problem hiding this comment.
Done
- Scope file-level #pragma warning disable CS0618 to only surround code that references obsolete types, leaving LINQ-based methods clean - Fix FilterClause/EqualToFilterClause/AnyTagEqualToFilterClause [Obsolete] messages to reference VectorSearchOptions<TRecord>.Filter (these types live in VectorData.Abstractions, not TextSearch)
Per reviewer feedback, replace file-level #pragma warning disable CS0618 with surgically scoped pragmas around only the code that references obsolete types (ITextSearch, TextSearchOptions, TextSearchFilter, FilterClause, ITextEmbeddingGenerationService). - 4 connectors (Bing/Brave/Google/Tavily): scope to class declaration, legacy ITextSearch methods region, and ExtractFiltersFromLegacy - VectorStoreTextSearch: scope to class declaration, obsolete constructors, legacy methods region, legacy ExecuteVectorSearchAsync, _textEmbeddingGeneration field, and BuildFilterExpression - VectorStoreTextSearchTests: scope to legacy test methods only, leaving LINQ-based generic interface tests clean
…ecuteVectorSearchCoreAsync
42e0b2e
into
microsoft:feature-text-search-linq
.Net: Address roji/westey review feedback round 2 on text search connectors (#10456)
Motivation and Context
Addresses second round of review feedback from @roji and westey (@westey-m) on PR #13384 (
feature-text-search-linq). This round focuses on code quality improvements: adding[Obsolete]attributes to legacy types, consolidating expression processing with pattern matching, and removing dead code.Issue: #10456
Description
Obsolete attributes
Marked the following legacy types as
[Obsolete]to signal the transition to LINQ-based filtering:FilterClause,EqualToFilterClause,AnyTagEqualToFilterClause(VectorData.Abstractions) — base filter clause hierarchy replaced by LINQ expressionsTextSearchFilter(SemanticKernel.Abstractions) — replaced byTextSearchOptions<TRecord>.FilterTextSearchOptions(non-generic,SemanticKernel.Abstractions) — replaced byTextSearchOptions<TRecord>Google connector refactoring
Consolidated 8 expression-processing methods into 3 using C# pattern matching:
ProcessFilterNode: Recursive switch-based handler for AND, equality, inequality, string Contains, null-guard, collection Contains (error), and negation patternsProcessNegatedFilterNode: Handles!(expr)patterns, mapping to Google'sexcludeTermsMapPropertyToGoogleFilter: Converted to expression-bodied memberTryProcessSingleExpression,TryProcessEqualityExpression,TryProcessInequalityExpression,TryProcessContainsExpression,TryProcessNotExpression,CollectAndCombineFilters,IsMemoryExtensionsContains,MapGoogleFilterToProperty(dead code)Bing/Brave/Tavily connector cleanup
switchexpressions replacingif-chainsMemoryExtensionstype checks — replaced withObject: nullfor static method detectionError handling improvements
NotSupportedExceptionwith actionable guidance whencollection.Contains(page.Property)is used (unsupported by these APIs)ExtractFiltersFromLegacy: All four connectors now throw on unsupportedFilterClausesubtypes instead of silently skippingpage.Property != nullcomparisons (silently skipped)Pragma consolidation
#pragma warning disable/restore CS0618pairs with single file-level pragmas across 22 files (library, tests, samples, integration tests)AgentWithTextSearchProvider.cswhere a local#pragma warning restore CS0618was re-enabling the warning mid-methodFiles Changed
Commit 1: Core changes (15 files, +198/-445)
FilterClause.cs[Obsolete]EqualToFilterClause.cs[Obsolete]AnyTagEqualToFilterClause.cs[Obsolete]TextSearchFilter.cs[Obsolete], file-level pragmaTextSearchOptions.cs[Obsolete]on non-generic classGoogleTextSearch.csBingTextSearch.csMemoryExtensions, collection Contains errorBraveTextSearch.csMemoryExtensions, file-level pragmaTavilyTextSearch.csMemoryExtensions, file-level pragmaVectorStoreTextSearch.csTextSearchExtensions.csTextSearchProviderOptions.csMockTextSearch.cs(UnitTests)MockTextSearch.cs(AotTests)VectorStoreTextSearchTests.csCommit 2: Pragma fixes for samples/tests (7 files, +16/-1)
Bing_RagWithTextSearch.cs#pragma warning disable CS0618Bing_FunctionCallingWithTextSearch.csBing_TextSearch.csGoogle_TextSearch.csTavily_TextSearch.csStep3_Search_With_FunctionCalling.csAgentWithTextSearchProvider.csContribution Checklist
Validation
dotnet format --verify-no-changes)dotnet build SK-dotnet.slnx -c Release --warnaserror --no-incremental)dotnet publish -f net10.0)