-
Notifications
You must be signed in to change notification settings - Fork 244
DRIVERS-2768 [Vector Search GA] Add support for types in search index creation #1541
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
Conversation
@@ -685,6 +685,16 @@ Common API Components | |||
*/ | |||
name: String; | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we just switched this spec to Markdown, so you'll have to move these changes to that file.
@@ -221,3 +221,88 @@ Case 6: Driver can successfully create and list search indexes with non-default | |||
- An index with the ``name`` of ``test-search-index-case6`` is present and the index has a field ``queryable`` with a value of ``true``. | |||
|
|||
#. Assert that ``index`` has a property ``latestDefinition`` whose value is ``{ 'mappings': { 'dynamic': false } }`` | |||
|
|||
Case 7: Driver can successfully handle search index types when creating indexes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the README file (actually I should have removed it in #1537)
Can you also add new spec tests that use |
We should also either update one of the existing prose tests or add a new one that explicitly uses |
The new prose test does explicitly create an index using |
The existing spec tests aren't run against an Atlas cluster, they're used as unit tests to verify the driver sends the correct command document. They expect an error telling the user to use Atlas for search support. I'm not sure adding more of the same tests that have the |
Perhaps just one extra test to make sure the field is handled appropriately? |
|
||
- An index with the ``name`` of ``test-search-index-case7-implicit`` is present and the index has a field ``queryable`` with a value of ``true``. | ||
|
||
5. Assert that ``index1`` has a property ``type`` whose value is ``search`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5. Assert that ``index1`` has a property ``type`` whose value is ``search`` | |
5. Assert that ``index1`` has a property ``type`` whose value is ``search``. |
|
||
- An index with the ``name`` of ``test-search-index-case7-explicit`` is present and the index has a field ``queryable`` with a value of ``true``. | ||
|
||
9. Assert that ``index2`` has a property ``type`` whose value is ``search`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9. Assert that ``index2`` has a property ``type`` whose value is ``search`` | |
9. Assert that ``index2`` has a property ``type`` whose value is ``search``. |
|
||
- An index with the ``name`` of ``test-search-index-case7-vector`` is present and the index has a field ``queryable`` with a value of ``true``. | ||
|
||
13. Assert that ``index3`` has a property ``type`` whose value is ``vectorSearch`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
13. Assert that ``index3`` has a property ``type`` whose value is ``vectorSearch`` | |
13. Assert that ``index3`` has a property ``type`` whose value is ``vectorSearch``. |
] | ||
}, | ||
{ | ||
"description": "create a vector search index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is not in the yml file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed!
Can you please install and run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Let's hold off on merging until the first two implementations are done.
@@ -664,6 +664,15 @@ interface IndexOptions { | |||
* @example For an index of name: 1, age: -1, the generated name would be "name_1_age_-1". | |||
*/ | |||
name: String; | |||
|
|||
/** | |||
* Optionally specify a type for the index. Defaults to "search" if not provided. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many other driver options are implemented such that we only send a value if the user explicitly provides one. How do you feel about mandating that driver MUST NOT send this field if not provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me if that's a consistent pattern among drivers. The server should default to search
implicitly if type
is not provided, so the behavior is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change be made to the CreateSearchIndexOptions
instead of IndexOptions
? https://github.com/mongodb/specifications/blob/master/source/index-management/index-management.md#common-interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateSearchIndexOptions
is currently unused. If drivers support adding type
to IndexOptions
, that seems to be a better fit as then the top-level fields of type
and name
are present in the same interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looking more closely it probably belongs in the SearchIndexModel
?
IndexOptions
is used for regular indexes, not search indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch! Yes, SearchIndexModel
seems like the correct place for type
.
@@ -664,6 +664,15 @@ interface IndexOptions { | |||
* @example For an index of name: 1, age: -1, the generated name would be "name_1_age_-1". | |||
*/ | |||
name: String; | |||
|
|||
/** | |||
* Optionally specify a type for the index. Defaults to "search" if not provided. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change be made to the CreateSearchIndexOptions
instead of IndexOptions
? https://github.com/mongodb/specifications/blob/master/source/index-management/index-management.md#common-interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POC passing in Node: mongodb/node-mongodb-native#4060
DRIVERS-2768
Please complete the following before merging:
clusters, and serverless).