Skip to content

DOCSP-46269: atlas search & atlas vector search pages #3255

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

Merged
merged 22 commits into from
Feb 10, 2025

Conversation

rustagir
Copy link
Contributor

@rustagir rustagir commented Jan 28, 2025

@rustagir rustagir requested a review from a team as a code owner January 28, 2025 21:16
@rustagir rustagir requested a review from norareidy January 28, 2025 21:16
@github-actions github-actions bot added the docs label Jan 28, 2025
@rustagir rustagir marked this pull request as draft January 28, 2025 21:26
@rustagir rustagir force-pushed the DOCSP-46269-atlas-search branch from a22decf to 403d10a Compare January 29, 2025 14:54
@rustagir rustagir marked this pull request as ready for review January 29, 2025 15:34
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.

Good work so far! A few formatting and wording suggestions

Comment on lines 251 to 253
- ``exact``: ``bool`` (default: ``false``)
- ``filter``: ``QueryInterface`` or ``array`` (default: no filtering)
- ``numCandidates``: ``int`` or ``null`` (default: ``null``)
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: Same as above. I think I would create a table here with descriptions

@rustagir rustagir requested a review from lindseymoore January 29, 2025 20:24
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be more readable to use a full name instead of this abbreviations I've never seen before?
atlas-search.txt

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 wanted to be comprehensive as Atlas Search and Atlas Vector Search are two separate features. But I can change the file name to atlas-search-vector-search

Copy link
Member

@GromNaN GromNaN Jan 29, 2025

Choose a reason for hiding this comment

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

Maybe create 2 pages that link each other. Indeed that's 2 distinct features.

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.

Thanks, LGTM w/ small suggestions!

Comment on lines 116 to 125
- ``index``: ``string``
- ``highlight``: ``array``
- ``concurrent``: ``bool``
- ``count``: ``string``
- ``searchAfter``: ``string``
- ``searchBefore`` ``string``
- ``scoreDetails``: ``bool``
- ``sort``: ``array``
- ``returnStoredSource``: ``bool``
- ``tracking``: ``array``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, perhaps listing the names of the parameters without the data type would suffice then, since the atlas documentation also includes the data type. The data type seems like unnecessary info if I don't know what the parameter is for. I'll leave up to you though!

@rustagir rustagir requested a review from GromNaN January 30, 2025 21:25
@rustagir rustagir changed the title DOCSP-46269: as & avs DOCSP-46269: atlas search & atlas vector search pages Jan 31, 2025
@rustagir rustagir requested a review from GromNaN February 7, 2025 20:08
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

2 remaining TODO, otherwise LGTM

@rustagir rustagir merged commit 9fdfbe5 into mongodb:5.x Feb 10, 2025
44 checks passed
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

While running tests on the project, I found some issues with those introduced by this PR. They were not executed because the phpunit group is missing. Fixes in #3279

* @runInSeparateProcess
* @preserveGlobalState disabled
*/
public function autocompleteSearchTest(): void
Copy link
Member

Choose a reason for hiding this comment

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

This test is not executed. In PHPUnit, test must be prefixed with test: testAutocompleteSearch

* @runInSeparateProcess
* @preserveGlobalState disabled
*/
public function vectorSearchTest(): void
Copy link
Member

Choose a reason for hiding this comment

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

Same issue with this test name: testVectorSearch

do {
$ready = true;
usleep(10_000);
foreach ($collection->listSearchIndexes() as $index) {
Copy link
Member

Choose a reason for hiding this comment

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

I get this error:

Undefined variable $collection

The variable is named $moviesCollection.

*/
public function vectorSearchTest(): void
{
$results = Book::vectorSearch(
Copy link
Member

Choose a reason for hiding this comment

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

The class is not imported, add use MongoDB\Laravel\Tests\Models\Book to the file.

$movies = Movie::search(
sort: ['title' => 1],
operator: Search::text('title', 'dream'),
)->get();
Copy link
Member

Choose a reason for hiding this comment

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

Movie::search returns an instance of Illuminate\Support\Collection. The method all() should be called to get all the results. get is to get a single item.

public function autocompleteSearchTest(): void
{
// start-auto-query
$movies = Movie::autocomplete('title', 'jak')->get();
Copy link
Member

Choose a reason for hiding this comment

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

Movie::search returns an instance of Illuminate\Support\Collection. The method all() should be called to get all the results. get is to get a single item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants