Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Atlas Search example #1147

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Add Atlas Search example #1147

merged 2 commits into from
Sep 13, 2023

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Aug 31, 2023 •
edited
Loading

Follow-up #1097

The example requires an Atlas M10+ cluster with Sample data loaded. It creates an index and performs a search query.

asciicast

@GromNaN GromNaN mentioned this pull request Aug 31, 2023
Merged
5 tasks
@GromNaN GromNaN marked this pull request as ready for review August 31, 2023 18:03
@GromNaN GromNaN force-pushed the search-example branch 2 times, most recently from e1d54bd to 9ea634c Compare August 31, 2023 18:43
jmikola
jmikola reviewed Aug 31, 2023
View reviewed changes
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

Should there be test for expected output, or is that intentionally excluded since it's redundant with our spec tests and not portable (although I suppose it could do the same check for an Atlas URI and skip accordingly)?

examples/atlas-search.php Outdated Show resolved Hide resolved
examples/atlas-search.php Outdated Show resolved Hide resolved
examples/atlas-search.php Show resolved Hide resolved
examples/atlas-search.php Outdated Show resolved Hide resolved
examples/atlas-search.php
/**
* This example demonstrates how to create a search index and perform a text search.
* It requires a MongoDB Atlas M10+ cluster with Sample Dataset loaded.
*/
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of a comment header explaining the purpose of the example. This might be a good place to come up with some header copypasta that notes supported environment variables and/or CLI arguments (down the line, not in this PR).

@GromNaN GromNaN force-pushed the search-example branch 2 times, most recently from edf25c8 to 5c8a818 Compare August 31, 2023 19:15
@GromNaN
Copy link
Member Author

GromNaN commented Aug 31, 2023

Should there be test for expected output, or is that intentionally excluded since it's redundant with our spec tests and not portable (although I suppose it could do the same check for an Atlas URI and skip accordingly)?

It would be nice, but that would be very slow to run because it needs to load the sample dataset and the output depends of the state of the cluster: if the index already exists or not.

@jmikola
Copy link
Member

jmikola commented Aug 31, 2023

It would be nice, but that would be very slow to run because it needs to load the sample dataset and the output depends of the state of the cluster: if the index already exists or not.

Makes sense. I suggest making a note of this in the final commit so there's some record that it was intentionally omitted.

GromNaN
GromNaN commented Sep 1, 2023
View reviewed changes
Copy link
Member Author

@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.

I added a test with a lot of skip conditions.

examples/atlas-search.php Show resolved Hide resolved
examples/atlas-search.php
if ($index->name === 'default') {
echo '.';

return $index->queryable;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a boolean, but I could not document it in phpdoc. #1097 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I see there's no Psalm error here. Is it worth casting this to a boolean and using bool return type hint for the callback? OK to leave this as-is if it doesn't matter.

@GromNaN
Copy link
Member Author

GromNaN commented Sep 6, 2023

Test can run in CI with #1150 config.

@GromNaN GromNaN requested a review from jmikola September 6, 2023 10:05
jmikola
jmikola approved these changes Sep 6, 2023
View reviewed changes
examples/atlas-search.php
if ($index->name === 'default') {
echo '.';

return $index->queryable;
Copy link
Member

Choose a reason for hiding this comment

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

I see there's no Psalm error here. Is it worth casting this to a boolean and using bool return type hint for the callback? OK to leave this as-is if it doesn't matter.

tests/ExamplesTest.php Outdated
$collection = $client->selectCollection('sample_airbnb', 'listingsAndReviews');
$count = $collection->estimatedDocumentCount();
if ($count === 0) {
$this->markTestSkipped('Atlas Search examples require the sample_airbnb database with the listingsAndReviews collection');
Copy link
Member

Choose a reason for hiding this comment

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

Do I assume correctly that this is for the benefit of running the test locally, and that it will most likely be skipped in CI because we don't load sample data?

I'm OK either way, but just wanted to ask.

Copy link
Member Author

Choose a reason for hiding this comment

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

We added the @group atlas, but I think I can insert some data to unit the collection in tests if it does not exist. We would benefit from having the test running in CI.

@GromNaN GromNaN force-pushed the search-example branch 2 times, most recently from fb90306 to ccb799b Compare September 6, 2023 20:05
@GromNaN GromNaN requested a review from jmikola September 6, 2023 20:21
jmikola
jmikola approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

Good idea loading some sample data for the benefit of CI.

GromNaN
GromNaN commented Sep 13, 2023
View reviewed changes
examples/atlas-search.php
foreach ($indexes as $index) {
if ($index->name === 'default') {
echo "The index already exists. Dropping it.\n";
$collection->dropSearchIndex($index->name);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is idempotent, I can remove surrounding loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact no, the error for NamespaceNotFound is silenced, not the error for IndexNotFound.
https://github.com/mongodb/specifications/blob/master/source/index-management/index-management.rst#namespacenotfound-errors

@GromNaN GromNaN merged commit 7589840 into mongodb:master Sep 13, 2023
44 checks passed
@GromNaN GromNaN deleted the search-example branch September 13, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alcaeus alcaeus alcaeus left review comments

@jmikola jmikola jmikola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GromNaN @jmikola @alcaeus