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

replace set with addFields in aggregations py #3111

Merged
merged 1 commit into from Jun 8, 2023

Conversation

AaronDou
Copy link

@AaronDou AaronDou commented May 25, 2023

What changes are proposed in this pull request?

This PR replaces the aggregation stage $set operator with the $addFields operator in aggregations.py. These two operators are alias to each other in MongoDB. The reason for the change is $addFields is supported by AWS DocumentDB while $set is not.

Note the $set operator is overloaded in MongoDB, and the one under change is the aggregation stage $set operator as opposed to the update operator $set.

After this PR is approved, follow up PRs will be submitted to replace all the $set aggregation operators to $addFields across the codebase to keep things consistent.

How is this patch tested? If it is not, please explain why.

pytest tests/unittests all green.
No new tests are added as this is effectively a cosmetic change.

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

@AaronDou AaronDou marked this pull request as ready for review May 25, 2023 01:42
@AaronDou AaronDou marked this pull request as draft May 25, 2023 01:47
@AaronDou AaronDou marked this pull request as ready for review May 25, 2023 01:52
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (7f5d87d) 48.61% compared to head (9e0f357) 48.61%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3111      +/-   ##
===========================================
- Coverage    48.61%   48.61%   -0.01%     
===========================================
  Files          227      227              
  Lines        33355    33359       +4     
  Branches       319      319              
===========================================
  Hits         16217    16217              
- Misses       17138    17142       +4     
Flag Coverage Δ
app 48.61% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ages/components/src/components/Results/Results.tsx 29.80% <0.00%> (-1.20%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

LGTM ✔️ @AaronDou note that I grabbed all the other instances of $set and $unset in #3174

@brimoor brimoor merged commit d7465aa into voxel51:develop Jun 8, 2023
9 of 11 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.

None yet

2 participants