Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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
rustagir merged 22 commits into mongodb:5.x from rustagir:DOCSP-46269-atlas-search
Feb 10, 2025

Conversation

@rustagir
Copy link
Contributor

@rustagir rustagir commented Jan 28, 2025 •
edited
Loading

@rustagir rustagir requested a review from a team as a code owner January 28, 2025 21:16
@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

@lindseymoore lindseymoore Jan 29, 2025

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 reacted with thumbs up emoji
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

GromNaN reacted with thumbs up emoji
Copy link
Member

@GromNaN GromNaN Jan 29, 2025 •
edited
Loading

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.

rustagir reacted with thumbs up emoji
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

@lindseymoore lindseymoore Jan 29, 2025

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 reacted with thumbs up emoji
@rustagir rustagir changed the title (削除) DOCSP-46269: as & avs (削除ここまで) (追記) DOCSP-46269: atlas search & atlas vector search pages (追記ここまで) Jan 31, 2025
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

rustagir reacted with thumbs up emoji
* @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

Reviewers

@GromNaN GromNaN GromNaN approved these changes

@lindseymoore lindseymoore lindseymoore approved these changes

@norareidy norareidy norareidy approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /