Skip to content

DOCS-14779 Add context around 5.1 let usage in geoNear #6073

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

ianf-mongodb
Copy link
Contributor

No description provided.

@ianf-mongodb ianf-mongodb force-pushed the DOCS-14779-geoNear-let-near-argument-allowed branch from ca66a80 to 0e5c11a Compare November 3, 2021 19:11
Copy link
Collaborator

@jeff-allen-mongo jeff-allen-mongo left a comment

Choose a reason for hiding this comment

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

Hey @ianf-mongodb , just a few small things to address prior to merge. Thanks!


.. _geoNear_let_example:

$geoNear with the ``let`` option
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the maximum and minimum distance examples should still be the first examples in this section. I'm worried that if we put the let option first, we're introducing a fairly abstract / complex example as the first one on the page. Let's make sure that the simplest, most straight-forward examples appear first when possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

In this example:

- The ``let`` option is used to set an array value of
``[-73.99279,40.719296]`` to the variable ``$pt``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the significance of the name "pt"? Is it short for "point", or is it an acronym of some kind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its just the variable name for the point array.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for clarifying. fwiw I don't love this as a variable name because it wasn't immediately clear to me what the significance of it was, but we can leave it alone.

Comment on lines 224 to 225
- ``$pt`` is a let option to the ``near`` parameter to the ``$geoNear``
stage.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This phrasing here is a little off.

Suggested change
- ``$pt`` is a let option to the ``near`` parameter to the ``$geoNear``
stage.
- ``$pt`` is specified as a let option to the ``near`` parameter in the ``$geoNear``
stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.


.. _geoNear_bounded_let_example:

$geoNear With Bound ``let`` Option
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
$geoNear With Bound ``let`` Option
$geoNear with Bound ``let`` Option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines 288 to 309
db.places.aggregate(
[
{
$lookup:
{
from: "places",
let: { pt: "$location" },
pipeline: [
{
$geoNear:
{
near: "$$pt",
distanceField: "distance"
}
}
],
as: "joinedField"
}
},
{ $match: { name: "Sara D. Roosevelt Park" } }
]
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The indentation here is a little off. We (generally) need the closing brackets to align with their opening counterparts. Suggest something like:

Suggested change
db.places.aggregate(
[
{
$lookup:
{
from: "places",
let: { pt: "$location" },
pipeline: [
{
$geoNear:
{
near: "$$pt",
distanceField: "distance"
}
}
],
as: "joinedField"
}
},
{ $match: { name: "Sara D. Roosevelt Park" } }
]
);
db.places.aggregate( [
{
$lookup: {
from: "places",
let: { pt: "$location" },
pipeline: [
{
$geoNear: {
near: "$$pt",
distanceField: "distance"
}
}
],
as: "joinedField"
}
},
{
$match: { name: "Sara D. Roosevelt Park" }
}
] );

Copy link
Contributor Author

@ianf-mongodb ianf-mongodb Nov 4, 2021

Choose a reason for hiding this comment

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

Updated.

}
)

The aggregation returns the following:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we explain a little bit what the results mean? The other examples on this page have explanations like:

The following aggregation uses $geoNear to find documents with a location at most 2 meters from the center [ -73.99279 , 40.719296 ] and category equal to Parks.

Users might have an easier time following these examples if we briefly explain what the goal of the pipeline we've created is.

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 specified this above each of the queries :

In this example, :pipeline:$lookup uses:

  • let to define $pt.
  • :pipeline:$geoNear in the pipeline.
  • $pt to define near in the :pipeline:$geoNear pipeline stage.

In this example:

  • The let option is used to set an array value of
    [-73.99279,40.719296] to the variable $pt.

  • $pt is specified as a let option to the near parameter in the
    $geoNear stage.

    Do you mean explictly changing "the aggregation returns the following" to describe the pipeline? I tried to copy the existing format as much as possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I saw, but my point was more that we've explained what the parameters are but not how they affect the end result. I think if we adjusted:

The aggregation returns the following:

To be something like:

The aggregation returns all documents with: 

- A location at most 2 meters from the point defined in  the ``let`` variable
- A ``category`` equal to ``Parks``.

.. code-block::

it could help paint a clearer picture. The examples which were on the page prior to this change do have an explanation like this. However, if you think the current approach is clear enough we can leave this alone.

@ianf-mongodb ianf-mongodb force-pushed the DOCS-14779-geoNear-let-near-argument-allowed branch from 0e5c11a to 12635b1 Compare November 4, 2021 12:40
Copy link
Contributor Author

@ianf-mongodb ianf-mongodb left a comment

Choose a reason for hiding this comment

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

Updated + responded.

@ianf-mongodb ianf-mongodb force-pushed the DOCS-14779-geoNear-let-near-argument-allowed branch 3 times, most recently from 31797fe to 82caaae Compare November 4, 2021 12:58
Copy link
Collaborator

@jeff-allen-mongo jeff-allen-mongo left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and comments @ianf-mongodb. Responded.

In this example:

- The ``let`` option is used to set an array value of
``[-73.99279,40.719296]`` to the variable ``$pt``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for clarifying. fwiw I don't love this as a variable name because it wasn't immediately clear to me what the significance of it was, but we can leave it alone.

Comment on lines 397 to 405
{
_id: ObjectId("61715cf9b0c1d171bb498fd7"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these objects need to be indented 1-2 more spaces? I'm wondering why the syntax highlighting isn't working in this code-block.

Copy link
Contributor Author

@ianf-mongodb ianf-mongodb Nov 4, 2021

Choose a reason for hiding this comment

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

Yes Corrected

}
)

The aggregation returns the following:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I saw, but my point was more that we've explained what the parameters are but not how they affect the end result. I think if we adjusted:

The aggregation returns the following:

To be something like:

The aggregation returns all documents with: 

- A location at most 2 meters from the point defined in  the ``let`` variable
- A ``category`` equal to ``Parks``.

.. code-block::

it could help paint a clearer picture. The examples which were on the page prior to this change do have an explanation like this. However, if you think the current approach is clear enough we can leave this alone.

@ianf-mongodb ianf-mongodb force-pushed the DOCS-14779-geoNear-let-near-argument-allowed branch 2 times, most recently from cd61d17 to e5da078 Compare November 4, 2021 18:41
@ianf-mongodb
Copy link
Contributor Author

ianf-mongodb commented Nov 4, 2021

Hey Jeff added the context to the example queries and corrected the code box so it is displaying the syntax highlighting correctly now. @jeff-allen-mongo

@ianf-mongodb ianf-mongodb force-pushed the DOCS-14779-geoNear-let-near-argument-allowed branch from e5da078 to e1f080f Compare November 4, 2021 18:55
@jeff-allen-mongo
Copy link
Collaborator

Thanks @ianf-mongodb , did you see this comment from my last review? I don't see the update for that one.

Copy link
Contributor Author

@ianf-mongodb ianf-mongodb left a comment

Choose a reason for hiding this comment

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

Committed suggestion.

@jeff-allen-mongo jeff-allen-mongo merged commit 911c14d into mongodb:master Nov 5, 2021
mongo-cr-bot pushed a commit that referenced this pull request Jan 30, 2024
* DOCSP-33168: Add missing sharding step

* rewording

* copy review
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.

2 participants