CSHARP-5882, CSHARP-5884, and CSHARP-5762#1900
CSHARP-5882, CSHARP-5884, and CSHARP-5762#1900ajcvickers wants to merge 4 commits intomongodb:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements support for storedSource configuration in vector search indexes, allowing users to specify which fields should be included or excluded from the Atlas vector index's stored source. The implementation deliberately avoids supporting true as a value (which would always fail for vector indexes, per the description).
Changes:
- Added
IncludedStoredFieldsandExcludedStoredFieldsproperties and aRenderStoredSourcehelper method toCreateVectorSearchIndexModelBase<TDocument>. - Added
WithIncludedStoredFieldsandWithExcludedStoredFieldsfluent methods (both field-definition and expression-based overloads) toCreateVectorSearchIndexModel<TDocument>andCreateAutoEmbeddingVectorSearchIndexModel<TDocument>, and updated theirRendermethods to emit thestoredSourceBSON. - Updated integration tests in
AtlasSearchIndexManagmentTests.csto verify the new stored source behavior, changed the poll period from 5s to 10s, and refactoredGetIndexesto acceptexpectTimeoutas an explicit parameter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/MongoDB.Driver/CreateVectorSearchIndexModelBase.cs |
Adds IncludedStoredFields, ExcludedStoredFields properties and RenderStoredSource helper; minor doc comment issue |
src/MongoDB.Driver/CreateVectorSearchIndexModel.cs |
Adds WithIncludedStoredFields/WithExcludedStoredFields fluent methods and calls RenderStoredSource in Render |
src/MongoDB.Driver/CreateAutoEmbeddingVectorSearchIndexModel.cs |
Same pattern as CreateVectorSearchIndexModel.cs for auto-embedding variant |
tests/MongoDB.Driver.Tests/Search/AtlasSearchIndexManagmentTests.cs |
Integration tests for new storedSource behavior; poll period, expectTimeout refactor, minor naming/formatting issues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
tests/MongoDB.Driver.Tests/Search/AtlasSearchIndexManagmentTests.cs
Outdated
Show resolved
Hide resolved
tests/MongoDB.Driver.Tests/Search/AtlasSearchIndexManagmentTests.cs
Outdated
Show resolved
Hide resolved
tests/MongoDB.Driver.Tests/Search/AtlasSearchIndexManagmentTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/MongoDB.Driver.Tests/Search/AtlasSearchIndexManagmentTests.cs:578
- Lines 570-572 assert that
numDimensions,similarity, andquantizationexist in theindexFielddocument with specific values, but lines 576-578 immediately assert those same fields do NOT exist (Should().Be(false)). This is a direct contradiction — both sets of assertions cannot be true at the same time. Lines 576-578 appear to be leftover from the "required only options" test (which copied the pattern of asserting optional fields are absent) and should be removed since this test explicitly configures all those options.
indexField.Contains("quantization").Should().Be(false);
indexField.Contains("numDimensions").Should().Be(false);
indexField.Contains("similarity").Should().Be(false);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
e81af22 to
8b0ebd1
Compare
|
Verified Evergreen failure not due to this PR. |
|
Assigned |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/MongoDB.Driver.Tests/Search/AtlasSearchIndexManagmentTests.cs:689
- In
Can_create_autoEmbed_vector_index_for_all_options, lines 681–685 assert thatindexField["numDimensions"],indexField["similarity"], andindexField["quantization"]exist with specific values, but then lines 687–689 immediately assert that the same fields do NOT exist (.Should().Be(false)). These assertions directly contradict each other and the test will always fail at the last three assertions (or would never actually validate the first set correctly). The three negative assertions at lines 687–689 appear to be leftover copy-paste from theCan_create_autoEmbed_vector_index_for_required_only_optionstest, which correctly validates the absence of those optional fields. They should be removed from the all-options test.
indexField.Contains("quantization").Should().Be(false);
indexField.Contains("numDimensions").Should().Be(false);
indexField.Contains("similarity").Should().Be(false);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
0fc36f8 to
ba7ec4d
Compare
| var quantizationValue = Quantization == VectorQuantization.BinaryNoRescore | ||
| ? "binaryNoRescore" | ||
| : Quantization?.ToString().ToLowerInvariant(); |
There was a problem hiding this comment.
CreateVectorSearchIndexModelBase is shared by both vector indexes and auto-embedding indexes and the same rendering logic here applies to both right? For auto-embedding indexes, rendering "none" is probably wrong based on the syntax document linked in the drivers ticket for the 5884 ticket. I suppose Atlas might an error in this case but not sure if the error is informative to the user, in which case we might have to throw ourselves and inform the server team to improve their error messaging. I'll defer to you to check this as I presume you have an environment set up for testing all this search changes.
There was a problem hiding this comment.
Good catch. Will fix.
| if (nestedRoot != null && options?.NestedFilter != null) | ||
| { | ||
| vectorSearchOperator.Add("filter", options.NestedFilter.Render( | ||
| (args with { SubPathRoot = nestedRoot }) with { RenderDollarForm = true })); | ||
| } |
There was a problem hiding this comment.
If a user sets NestedFilter but the vector field path is NOT nested (e.g., "Embedding" instead of "Plot.Embedding"), this condition fails silently and the filter gets ignored with no warning. Should we make sure NestedFilter can only be used when the vector field path is nested? Or does the server throw an informative error in this case?
There was a problem hiding this comment.
Yeah, it would be better to explicitly throw in this case. Will fix.
| params FieldDefinition<TDocument>[] filterFields) | ||
| : base(name, SearchIndexType.VectorSearch) | ||
| { | ||
| Field = field; |
There was a problem hiding this comment.
Should this field parameter be validated to not be null? I know it doesn't makes sense for anyone using this to ever pass a null here but it doesn't hurt to be defensive. In any case, I'll defer to your judgement.
Allows fields to be included or excluded. Does not allow `true` to be set because this will always fail for a vector index.
ba7ec4d to
ff3b660
Compare
CSHARP-5882: Support stored source in vector indexes
Allows fields to be included or excluded. Does not allow
trueto be set because this will always fail for a vector index.CSHARP-5884: Add new fields for Auto embedding in Atlas Vector search indexes
CSHARP-5762: Vector search against nested embeddings and arrays of embeddings