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.

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

DOCSP-41146: agg exp operations #32

Merged
merged 12 commits into from
Aug 14, 2024

Conversation

rustagir
Copy link
Collaborator

@rustagir rustagir commented Aug 7, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-41146
Staging - https://preview-mongodbrustagir.gatsbyjs.io/kotlin-sync/DOCSP-41146-agg-exp-operations/agg-exp-ops/

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
    WARNING(aggregation/agg-exp-ops.txt:0ish): Page not included in any toctree and not marked :orphan:
    Will be fixed by DOCSP-41145: Aggregation landing page #30

  • Did you run a spell-check?

  • Did you run a grammar-check?

  • Are all the links working?

  • Are the facets and meta keywords accurate?

Copy link

netlify bot commented Aug 7, 2024

👷 Deploy request for docs-kotlin-sync pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 398bcbe

Copy link
Collaborator

@mcmorisi mcmorisi left a comment

Choose a reason for hiding this comment

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

Great start on this one – this is a beefy page and I think there's some things we can do to trim it down for the sake of readers. Happy to keep an open discussion for any points of feedback.

source/aggregation/agg-exp-ops.txt Outdated Show resolved Hide resolved
source/aggregation/agg-exp-ops.txt Outdated Show resolved Hide resolved
source/aggregation/agg-exp-ops.txt Outdated Show resolved Hide resolved
source/aggregation/agg-exp-ops.txt Outdated Show resolved Hide resolved
- ``group(<expression>)``

.. TODO To learn more about these methods, see the
.. :ref:`kotlin-sync-sync-aggregation`.
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
.. :ref:`kotlin-sync-sync-aggregation`.
.. :ref:`kotlin-sync-aggregation`.

Comment on lines 398 to 399
``getArray()`` method if you work with the values of the
array as their specific type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Little unclear on the meaning of the "if you work with the values of the array as their specific type". Is there any way to reword this?

readings (in degrees Fahrenheit) as extreme.

The ``or()`` operator checks to see if temperatures are extreme by comparing
the ``temperature`` field to predefined values with ``lt()`` and ``gt()``.
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
the ``temperature`` field to predefined values with ``lt()`` and ``gt()``.
the ``temperature`` field to predefined values by using ``lt()`` and ``gt()``.

Comment on lines 417 to 419
the ``totalSeats`` and ``isAvailable`` variables. If you don't pull
out these intermediary values into variables, the code still produces
equivalent results.
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
the ``totalSeats`` and ``isAvailable`` variables. If you don't pull
out these intermediary values into variables, the code still produces
equivalent results.
the ``totalSeats`` and ``isAvailable`` variables. If you don't assign
these intermediary values to variables, the code still produces
equivalent results.

Comment on lines 678 to 684
One advantage of using the ``passTo()`` method is that you can reuse
your custom methods for other aggregations. You could
use the ``gradeAverage()`` method to find the average of grades for
groups of students filtered by entry year or district, not just their
class, for example. You could use the ``evaluate()`` method to evaluate, for
example, an individual student's performance, or an entire school's or
district's performance.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion to resolve some duplicative wording

Suggested change
One advantage of using the ``passTo()`` method is that you can reuse
your custom methods for other aggregations. You could
use the ``gradeAverage()`` method to find the average of grades for
groups of students filtered by entry year or district, not just their
class, for example. You could use the ``evaluate()`` method to evaluate, for
example, an individual student's performance, or an entire school's or
district's performance.
One advantage of using the ``passTo()`` method is that you can reuse
your custom methods for other aggregations. For example, you could
use the ``gradeAverage()`` method to find the average of grades for
groups of students filtered by entry year or district, not just their
class. Likewise, you could use the ``evaluate()`` method to evaluate
an individual student's performance or an entire school's performance.

Comment on lines 835 to 838
The ``dayOfWeek()`` method determines which day of the week it is and converts
it to a number based on which day is a Monday according to the
``"America/New_York"`` parameter. The ``eq()`` method compares this value to
``2``, which corresponds to Monday based on the provided timezone parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a little implementation-specific – I think this would serve readers better if it focused on the outcome of using the method.

@rustagir rustagir requested a review from mcmorisi August 8, 2024 18:59
Copy link
Collaborator

@mcmorisi mcmorisi left a comment

Choose a reason for hiding this comment

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

LGTM, I don't think any feedback I left here is blocking, but there are a couple of callouts/questions.

Comment on lines 190 to 202
the Server manual documentation. While each method is effectively
equivalent to the corresponding Query API expression, they may differ in
expected parameters and implementation.

The example in each section uses the ``listOf()`` method to create a
pipeline from the aggregation stage. Then, each example passes the
pipeline to the ``aggregate()`` method of ``MongoCollection``.

.. note::

The driver generates a Query API expression that may be different
from the Query API expression provided in each example. However,
both expressions will produce the same aggregation result.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Is the last sentence in the first highlighted paragraph redundant with the highlighted note?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont think so, I think paragraph sentence is talking about individual operators, while the note is talking about the completed expression, there is a typo though that i will fix (expression -> operator in the paragraph)

average precipitation, in millimeters, for each month.

The ``multiply()`` operator multiplies the ``precipitation`` field by
``25.4`` to convert the value to millimeters. The ``avg()`` accumulator method
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
``25.4`` to convert the value to millimeters. The ``avg()`` accumulator method
``25.4`` to convert the field value to millimeters. The ``avg()`` accumulator method

Comment on lines 419 to 421
the ``totalSeats`` and ``isAvailable`` variables. If you don't assign
these intermediary values to variables, the code still produces
equivalent results.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is rendering oddly on the staging site

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks ok to me!

Comment on lines 959 to 962
.. tip::

Represent data as a map if the data maps
arbitrary keys such as dates or item IDs to values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about the purpose of this page, is this tip necessary? This seems more relevant for a page about how to structure a data schema, as opposed to performing aggregation operations. We also have a pretty thorough example usecase later on in the section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can remove

@rustagir rustagir requested a review from rozza August 13, 2024 15:00
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM!

Thats an epic page, I haven't used the Mql code before, so I learn't lots to!

@rustagir rustagir merged commit 7ee3261 into mongodb:master Aug 14, 2024
1 of 2 checks passed
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