Skip to content

DOCSP-48420: bson v2.14 vector support#169

Merged
rustagir merged 7 commits intomongodb:masterfrom
rustagir:DOCSP-48420-bson-vector-support
Mar 19, 2025
Merged

DOCSP-48420: bson v2.14 vector support#169
rustagir merged 7 commits intomongodb:masterfrom
rustagir:DOCSP-48420-bson-vector-support

Conversation

@rustagir
Copy link
Contributor

@rustagir rustagir commented Mar 14, 2025

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-48420
Staging:

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

@netlify
Copy link

netlify bot commented Mar 14, 2025

Deploy Preview for docs-rust ready!

Name Link
🔨 Latest commit cc7554c
🔍 Latest deploy log https://app.netlify.com/sites/docs-rust/deploys/67dad6be0675f300077a0db4
😎 Deploy Preview https://deploy-preview-169--docs-rust.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rustagir rustagir requested review from a team and isabelatkinson and removed request for a team March 14, 2025 15:45
Copy link
Collaborator

@lindseymoore lindseymoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions and suggestions, thanks!

* - ``filter``
- document
- Specifies a pre-filter for documents to search on
- No filtering
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- No filtering
- ``null`` (No filtering)

S: Is there a default value you can list along with the default value behavior?

Copy link
Contributor Author

@rustagir rustagir Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came from the Atlas docs, which didn't supply default values. I'm not sure null is actually the type interpreted by the server

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be worth asking the tech reviewer for the default value/clarification

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave these as-is; I'm not sure how the server represents these values when they're unset, but that shouldn't be of concern to the user and I think this phrasing better represents the actual behavior. There's no default value from the driver's perspective because the driver just sends the pipeline the user provided directly to the server.

- number
- Specifies the number of nearest neighbors to use during the
search
- No limit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, is there a default value you can list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See reply above

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would ask tech reviewer

at the ``crates.io`` crate registry.

By implementing functionality from the ``serde`` crate into your
application, you can use custom Rust types such as structs and enums
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
application, you can use custom Rust types such as structs and enums
application, you can use custom Rust types such as ``structs`` and ``enums``

S: Should this be monospaced since they're types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By implementing functionality from the ``serde`` crate into your
application, you can use custom Rust types such as structs and enums
to model your data.
to model your data and Rust data types to model your field values.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: I'm confused by this. What is the difference between data and field values? Can you make a clearer distinction?

Copy link
Contributor Author

@rustagir rustagir Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that you can use a struct to model a whole document and select Rust data types for each field value - will revert back to old wording if this is already clear

Comment on lines 98 to 106
.. note:: Query Vector Length

For demonstrative purposes, the examples in this section use
sample query vectors that contain very few elements, compared to
the query vector you might use in a runnable application. To view an
example that contains the full-length query vector, see the
:atlas:`Atlas Vector Search Quick Start </atlas-vector-search/tutorials/vector-search-quick-start/>`
and select :guilabel:`Rust` from the :guilabel:`Select your language` dropdown in the upper-right
corner of the page.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is the example still runnable without the correct query vector? If not, the output might be misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a code comment to direct users to replace the query vector

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S: I know it looks ugly to have the long vector, but it might be worth adding it to the page rather than including a note explaining why you didn't include and how to get the longer vector. I'll leave up to your discretion!

Comment on lines 175 to 176
.. list-table::
:header-rows: 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.. list-table::
:header-rows: 1
.. list-table::
:widths: 20 20 40 20
:header-rows: 1

S: Edit column widths, so "document" doesn't wrap


To learn more about these parameters, see the :atlas:`Fields
</atlas-vector-search/vector-search-stage/#fields>` section of the
``$vectorSearch`` operator reference in the Atlas documentation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you include an Additional Information section with API refs?

@rustagir rustagir requested a review from lindseymoore March 18, 2025 14:26
Copy link
Collaborator

@lindseymoore lindseymoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, otherwise LGTM! Let me know if you have any questions

Comment on lines 100 to 102
For demonstrative purposes, the examples in this section use
sample query vectors that contain very few elements, compared to
the query vector you use in a runnable application.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For demonstrative purposes, the examples in this section use
sample query vectors that contain very few elements, compared to
the query vector you use in a runnable application.
For demonstrative purposes, the example ``query_vector`` in this section
contains very few elements, compared to
the query vector you might use in a runnable application.

S: Just to make it clear what example vector you are referring to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the full query vector so removing this admonition

Copy link
Collaborator

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small formatting suggestion, otherwise looks good!

Comment on lines 32 to 37
"queryVector": &query_vector,
"path": "plot_embedding",
"numCandidates": 150,
"index": "vector_index",
"limit": 5
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest indenting these lines for better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran rustfmt and this was the indentation that was implemented - should I add an extra indent?

Comment on lines 58 to 63
"queryVector": &query_vector,
"path": "plot_embedding",
"numCandidates": 150,
"index": "vector_index",
"limit": 5
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here

@rustagir rustagir merged commit aeb0adc into mongodb:master Mar 19, 2025
rustagir added a commit that referenced this pull request Mar 19, 2025
rustagir added a commit that referenced this pull request Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants