.Net: Remove legacy filtering from MEVD providers#13636
Conversation
b764d98 to
90c71ba
Compare
90c71ba to
ab0aa44
Compare
ab0aa44 to
abdbad6
Compare
abdbad6 to
c2219a0
Compare
There was a problem hiding this comment.
Pull request overview
Removes legacy VectorSearchFilter/OldFilter support across MEVD (.NET VectorData) connectors and tests, standardizing filtering on the newer VectorSearchOptions.Filter expression-based API.
Changes:
- Deleted
VectorSearchFilterand removedOldFilterfrom search option types. - Removed legacy-filter translation code from several connectors (Weaviate, Redis, Qdrant, CosmosNoSql, MongoDB, PgVector, SqliteVec, AzureAISearch).
- Updated/removed unit + conformance tests that exercised legacy filtering, and adjusted a few expected query strings for the new filter path.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/test/VectorData/Weaviate.UnitTests/WeaviateQueryBuilderTests.cs | Removes Weaviate legacy-filter query tests and related helper type. |
| dotnet/test/VectorData/VectorData.ConformanceTests/FilterTests.cs | Removes legacy filter conformance tests and helper method. |
| dotnet/test/VectorData/SqliteVec.ConformanceTests/SqliteFilterTests.cs | Removes overrides for legacy filter tests. |
| dotnet/test/VectorData/SqlServer.ConformanceTests/SqlServerFilterTests.cs | Removes skipped legacy-filter overrides. |
| dotnet/test/VectorData/Redis.UnitTests/RedisJsonCollectionTests.cs | Switches Redis search test from OldFilter to Filter and updates expected query string quoting. |
| dotnet/test/VectorData/Redis.UnitTests/RedisHashSetCollectionTests.cs | Switches Redis hash search test from OldFilter to Filter and updates expected query string quoting. |
| dotnet/test/VectorData/Redis.UnitTests/RedisCollectionSearchMappingTests.cs | Removes Redis legacy filter mapping tests. |
| dotnet/test/VectorData/Redis.ConformanceTests/RedisFilterTests.cs | Removes Redis legacy filter overrides. |
| dotnet/test/VectorData/Qdrant.UnitTests/QdrantCollectionTests.cs | Switches Qdrant search test from OldFilter to Filter. |
| dotnet/test/VectorData/Qdrant.UnitTests/QdrantCollectionSearchMappingTests.cs | Removes Qdrant legacy filter mapping tests. |
| dotnet/test/VectorData/Pinecone.ConformanceTests/PineconeFilterTests.cs | Removes Pinecone legacy filter overrides. |
| dotnet/test/VectorData/PgVector.ConformanceTests/PostgresFilterTests.cs | Removes PgVector legacy filter override. |
| dotnet/test/VectorData/MongoDB.UnitTests/MongoCollectionSearchMappingTests.cs | Deletes MongoDB legacy filter mapping tests file. |
| dotnet/test/VectorData/MongoDB.ConformanceTests/MongoFilterTests.cs | Removes MongoDB legacy filter overrides. |
| dotnet/test/VectorData/CosmosNoSql.UnitTests/CosmosNoSqlCollectionQueryBuilderTests.cs | Removes Cosmos NoSql legacy-filter query tests and updates call sites. |
| dotnet/test/VectorData/CosmosMongoDB.UnitTests/CosmosMongoCollectionSearchMappingTests.cs | Deletes CosmosMongo legacy filter mapping tests file. |
| dotnet/test/VectorData/CosmosMongoDB.ConformanceTests/CosmosMongoFilterTests.cs | Removes CosmosMongo legacy filter overrides. |
| dotnet/test/VectorData/AzureAISearch.UnitTests/AzureAISearchCollectionTests.cs | Switches Azure AI Search tests from OldFilter to Filter and updates expected filter string format. |
| dotnet/src/VectorData/Weaviate/WeaviateQueryBuilder.cs | Removes legacy filter switch + builder; uses Filter only. |
| dotnet/src/VectorData/VectorData.Abstractions/VectorSearch/VectorSearchFilter.cs | Deletes obsolete VectorSearchFilter type. |
| dotnet/src/VectorData/VectorData.Abstractions/VectorSearch/RecordSearchOptions.cs | Removes OldFilter property and updates XML docs. |
| dotnet/src/VectorData/VectorData.Abstractions/VectorSearch/HybridSearchOptions.cs | Removes OldFilter property. |
| dotnet/src/VectorData/SqliteVec/SqliteCollection.cs | Removes legacy-filter translation branch and helper. |
| dotnet/src/VectorData/SqlServer/SqlServerCollection.cs | Removes explicit legacy-filter rejection checks. |
| dotnet/src/VectorData/Redis/RedisCollectionSearchMapping.cs | Removes legacy filter building and related helper. |
| dotnet/src/VectorData/Qdrant/QdrantCollectionSearchMapping.cs | Removes legacy filter mapping helper. |
| dotnet/src/VectorData/Qdrant/QdrantCollection.cs | Removes legacy filter switch; uses Filter only. |
| dotnet/src/VectorData/Pinecone/PineconeCollectionSearchMapping.cs | Deletes Pinecone legacy mapping helper file. |
| dotnet/src/VectorData/Pinecone/PineconeCollection.cs | Removes legacy filter switch; uses Filter only. |
| dotnet/src/VectorData/PgVector/PostgresSqlBuilder.cs | Removes legacy filter handling and renames “new filter” helpers to generic filter helpers. |
| dotnet/src/VectorData/PgVector/PostgresCollection.cs | Stops passing legacy filter into SQL builder. |
| dotnet/src/VectorData/MongoDB/MongoCollectionSearchMapping.cs | Removes legacy filter builder. |
| dotnet/src/VectorData/MongoDB/MongoCollection.cs | Removes legacy filter switch; uses Filter only. |
| dotnet/src/VectorData/InMemory/InMemoryCollectionSearchMapping.cs | Removes legacy in-memory filtering implementation. |
| dotnet/src/VectorData/InMemory/InMemoryCollection.cs | Removes legacy filter switch and uses expression filter only. |
| dotnet/src/VectorData/CosmosNoSql/CosmosNoSqlCollectionQueryBuilder.cs | Removes legacy filter parameter + builder; uses translator for Filter only. |
| dotnet/src/VectorData/CosmosNoSql/CosmosNoSqlCollection.cs | Stops passing legacy filter into query builder. |
| dotnet/src/VectorData/CosmosMongoDB/CosmosMongoCollectionSearchMapping.cs | Removes legacy filter builder. |
| dotnet/src/VectorData/CosmosMongoDB/CosmosMongoCollection.cs | Removes legacy filter switch; uses translator for Filter only. |
| dotnet/src/VectorData/AzureAISearch/AzureAISearchCollectionSearchMapping.cs | Deletes legacy filter mapping helper file. |
| dotnet/src/VectorData/AzureAISearch/AzureAISearchCollection.cs | Stops propagating OldFilter and builds filter from Filter only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dotnet/src/VectorData/VectorData.Abstractions/VectorSearch/RecordSearchOptions.cs
Show resolved
Hide resolved
dotnet/src/VectorData/VectorData.Abstractions/VectorSearch/RecordSearchOptions.cs
Show resolved
Hide resolved
dotnet/test/VectorData/AzureAISearch.UnitTests/AzureAISearchCollectionTests.cs
Show resolved
Hide resolved
dotnet/test/VectorData/AzureAISearch.UnitTests/AzureAISearchCollectionTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 91%
✓ Correctness
This PR cleanly removes the deprecated VectorSearchFilter-based legacy filter infrastructure (OldFilter property, VectorSearchFilter class, and all provider-specific legacy filter building methods) across all vector data providers, keeping only the newer Expression<Func<TRecord, bool>>-based Filter property. All provider implementations are consistently updated: the switch-based dual-filter resolution is replaced with simple null-check conditionals, legacy filter helper methods and their tests are removed, and remaining tests are updated to use the new expression-based Filter. The FilterClause/EqualToFilterClause/AnyTagEqualToFilterClause types are correctly left in place since they're still used by TextSearchFilter in the SemanticKernel.Abstractions project. The PostgresSqlBuilder correctly makes the renamed Translate* methods private since no tests call them directly. Test assertion values are updated to reflect minor differences in filter string formatting between the old and new translators (e.g., added parentheses in OData, proper quoting in Redis tag filters), which are semantically equivalent.
✓ Security Reliability
This PR cleanly removes the deprecated VectorSearchFilter (legacy filter) infrastructure across all vector data providers, consolidating on the type-safe Expression<Func<TRecord, bool>> filter approach. The changes are consistent across all providers (AzureAISearch, CosmosMongoDB, CosmosNoSql, InMemory, MongoDB, PgVector, Pinecone, Qdrant, Redis, SqlServer, SqliteVec, Weaviate). From a security perspective, this is a net positive: the removed legacy filter code used string interpolation/concatenation to build query filters (e.g., OData strings, Redis filter strings) from user-provided values, which could be susceptible to injection. The new expression-based translators use parameterized queries and structured translation, which is inherently safer. No resource leaks, secrets, or unsafe deserialization patterns were introduced. The PostgresSqlBuilder correctly changed the renamed methods from internal to private, reducing API surface. All test updates are consistent with the production changes.
✗ Test Coverage
This PR removes the legacy
VectorSearchFilter/OldFilterAPI from all vector data providers, keeping only the Expression-basedFilterproperty. The test updates are mostly correct—tests that usedOldFilterwere either converted to useFilterexpressions or deleted along with the legacy code they tested. However, several unit test files lost significant filter-related test coverage without replacement: CosmosNoSqlCollectionQueryBuilderTests lost 4 of 5 tests (filter with WHERE clause, filter+offset, invalid filter, hybrid+filter), WeaviateQueryBuilderTests lost 3 tests (filter query generation, invalid filter value, non-existent property), QdrantCollectionSearchMappingTests lost all legacy filter tests, and MongoCollectionSearchMappingTests and CosmosMongoCollectionSearchMappingTests were deleted entirely. While conformance tests cover Expression-based filtering at the integration level, they don't replace the deleted unit tests that verified query/filter string generation at the builder level. The CosmosNoSql and Weaviate providers have zero filter translator unit tests.
✓ Design Approach
This PR cleanly removes the deprecated
VectorSearchFilter/OldFilterAPI across all connectors and the abstractions layer, consolidating on theExpression<Func<TRecord, bool>>filter approach. The approach is correct and well-executed: all legacy code paths are deleted, tests are updated to use the new API, and the filter clause types (FilterClause,EqualToFilterClause,AnyTagEqualToFilterClause) are intentionally retained because they are still used in theVectorStoreTextSearchbackward-compat layer. TheinternalPostgres helper methods (GenerateNewFilterWhereClause/GenerateNewFilterCondition) are correctly madeprivatesince no tests in theInternalsVisibleToassembly reference them. The behavior changes in generated filter strings (e.g., Redis quoting, AzureAISearch parenthesization) reflect the new expression translator's output and are validated by updated tests. No design issues found.
Flagged Issues
- CosmosNoSqlCollectionQueryBuilderTests: 4 tests removed (filter with WHERE clause, filter+offset, invalid filter, hybrid+filter) with no Expression-based replacements. Only 1 trivial no-filter test remains, leaving zero unit test coverage for the query builder's filter integration with CosmosNoSqlFilterTranslator.
- WeaviateQueryBuilderTests: 3 tests removed (filter query generation, invalid filter value, non-existent property) with no Expression-based replacements. Filter clause generation in the GraphQL query builder has zero unit test coverage.
Suggestions
- The FilterClause, EqualToFilterClause, and AnyTagEqualToFilterClause types are now only kept alive by the VectorStoreTextSearch backward-compat layer. Consider deprecating/removing them in a follow-up, or adding an explicit comment on each type documenting this dependency so a future cleanup of the TextSearch compat code doesn't orphan them.
- The PostgresSqlBuilder.cs doc comment (line 471 post-diff) still says 'Generates filter clause from the provided filter' but could clarify that it delegates to the expression-based PostgresFilterTranslator.
- Add replacement unit tests in CosmosNoSqlCollectionQueryBuilderTests that call BuildSearchQuery with an Expression-based filter (e.g.,
filter: (DummyType x) => x.TestProperty2 == "test-value-2") and assert the generated WHERE clause and parameters. - Add replacement unit tests in WeaviateQueryBuilderTests that call BuildSearchQuery with an Expression-based filter and assert the generated GraphQL
whereclause. - Consider adding dedicated CosmosNoSqlFilterTranslatorTests and WeaviateFilterTranslatorTests (similar to existing PostgresFilterTranslatorTests and RedisFilterTranslatorTests) to unit-test the expression-to-query translation in isolation.
- The QdrantCollectionTests.CanSearchWithVectorAndFilterAsync test now uses
Filter = r => r.Data == "data 1", but the mock assertion still checks for a legacy-style Qdrant Filter with Must conditions. Verify this works correctly with the new QdrantFilterTranslator path, as the translator may produce a different Filter structure than the removed BuildFromLegacyFilter method.
Automated review by roji's agents
Removes all legacy filtering logic from MEVD, leaving only (obsoleted) FilterClause and subclasses, which are still needed by ITextSearch (see conversation in #13384 (comment)).
As suggested by @westey-m let's do an SK release before merging this (see above).
Closes #10456
/cc @alzarei