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

feat(ingestion/mongodb) re-order aggregation logic #12428

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Haebuk
Copy link

@Haebuk Haebuk commented Jan 22, 2025

I fix the linked issue by reordering aggregation logics.
It would be sampling first then measuring doc size and filtering so much faster.

Checklist

Remove an unnecessary comma in the MongoDB aggregation logic to ensure consistency in the code style.
This change does not affect functionality but improves code readability.
Refactor the MongoDB document aggregation process by consolidating the aggregation call into a single location, regardless of the sample size condition. This change enhances code clarity and maintains functionality.
Refactor the MongoDB document aggregation logic to streamline the handling of sampling conditions. The changes ensure that the aggregation process is clearer by consolidating the logic for random sampling and limiting sample size into a more cohesive structure, enhancing code readability while maintaining existing functionality.
Reorganize the logic for adding a document size filter in the MongoDB aggregation process. The changed aggregation order improves MongoDB scanning performance.
@github-actions github-actions bot added ingestion PR or Issue related to the ingestion of metadata community-contribution PR or Issue raised by member(s) of DataHub Community labels Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
...-ingestion/src/datahub/ingestion/source/mongodb.py 91.01% <100.00%> (+1.56%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de55788...2debac9. Read the comment docs.

@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Jan 22, 2025
@Haebuk Haebuk changed the title Feature/source mongodb aggregation logic feat(ingestion/mongodb) re-order aggregation logic Jan 23, 2025
…ndom sampling

- Introduced a new golden JSON file for MongoDB ingestion without random sampling.
- Updated the test suite to include a pipeline that verifies the ingestion process.
- Ensured the output is validated against the new golden file to maintain data integrity.

This commit enhances the testing framework for MongoDB ingestion, ensuring that the functionality works as expected without random sampling.
@datahub-cyborg datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Jan 29, 2025
…ormance

- Added a comment in the `construct_schema_pymongo` function to clarify that the order of aggregations affects execution time.
- Explained that placing the sample/limit aggregation first allows subsequent aggregations to process a smaller dataset, thus improving performance.
@datahub-cyborg datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Feb 3, 2025
@datahub-cyborg datahub-cyborg bot added merge-pending-ci A PR that has passed review and should be merged once CI is green. and removed needs-review Label for PRs that need review from a maintainer. labels Feb 3, 2025
@Haebuk
Copy link
Author

Haebuk commented Feb 4, 2025

This is my first pull request, and I want to ensure everything is in order for it to be merged. Could you please let me know if there are any additional steps or requirements I need to fulfill? I appreciate your guidance and support.
Thank you!

@sgomezvillamor
Copy link
Contributor

We can ignore codecov complains and this can be merged. Thanks for the contrib!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community ingestion PR or Issue related to the ingestion of metadata merge-pending-ci A PR that has passed review and should be merged once CI is green.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants