Implement `$unset` aggregation pipeline stage #2676

shibasisp · 2023-05-21T18:05:22Z

Description

Closes #1432 .

Implemented $unset aggregation pipeline. Currently this works with:

single field
multiple fields (array);
dot notation support for both.

Readiness checklist

I added/updated unit tests.
I added/updated integration/compatibility tests.
I added/updated comments and checked rendering.
I made spot refactorings.
I updated user documentation.
I ran task all, and it passed.
I ensured that PR title is good enough for the changelog.
(for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
I marked all done items in this checklist.

CLAassistant · 2023-05-21T18:05:27Z

All committers have signed the CLA.

codecov · 2023-05-21T18:11:00Z

Codecov Report

Merging #2676 (fe5cf9d) into main (89d6562) will increase coverage by 0.08%.
The diff coverage is 92.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2676      +/-   ##
==========================================
+ Coverage   63.91%   64.00%   +0.08%     
==========================================
  Files         442      443       +1     
  Lines       22602    22658      +56     
==========================================
+ Hits        14447    14502      +55     
- Misses       7207     7208       +1     
  Partials      948      948

Impacted Files	Coverage Δ
...rnal/handlers/common/aggregations/stages/stages.go	`29.03% <ø> (ø)`
internal/handlers/commonerrors/error.go	`80.00% <ø> (ø)`
internal/handlers/commonerrors/errorcode_string.go	`80.00% <ø> (ø)`
...ernal/handlers/common/aggregations/stages/unset.go	`92.85% <92.85%> (ø)`

... and 1 file with indirect coverage changes

Flag	Coverage Δ
integration	`57.08% <92.85%> (+0.10%)`	⬆️
mongodb	`5.00% <0.00%> (-0.02%)`	⬇️
pg	`57.00% <92.85%> (+0.10%)`	⬆️
shard-1	`44.32% <0.00%> (+0.47%)`	⬆️
shard-2	`42.62% <0.00%> (-1.25%)`	⬇️
shard-3	`43.36% <92.85%> (+1.02%)`	⬆️
unit	`25.04% <0.00%> (-0.11%)`	⬇️

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

rumyantseva

Thank you very much for contributing!

I see the PR is a draft, but the review was requested, so I leave some comments.

I like the implementation, but I suggest making some additions in tests (see my comments), so we can reveal some problems or errors that need special handling.

Could you also run task lint please? We have some specific rules to format code, it helps us keep the code consistent.

internal/handlers/common/aggregations/stages/unset.go

integration/aggregate_documents_compat_test.go

internal/handlers/common/aggregations/stages/unset.go

AlekSi · 2023-05-29T08:36:25Z

@shibasisp Any news? Anything we could help with?

shibasisp · 2023-06-01T06:01:24Z

Apologies for the delay. @AlekSi I will be addressing the comments by this weekend.

mergify · 2023-06-02T09:27:16Z

@shibasisp this pull request has merge conflicts.

AlekSi

It does not compile

rumyantseva

LGTM. Thanks for contributing!

noisersup

Thanks for your contribution! The logic here looks good to me. I would only suggest to add a comment as it's not clear at the first glance that ProjectionDocument would use exclusion and not inclusion for sure.

Also we might add 3 test cases for $unset field to make a code coverage slightly better:

empty array
empty string
array with empty string

internal/handlers/common/aggregations/stages/unset.go

rumyantseva

Let's add the last couple of tests to cover error codes fully!

internal/handlers/common/aggregations/stages/unset.go

rumyantseva

Let's add the last couple of tests to cover error codes fully!

internal/handlers/common/aggregations/stages/unset.go

chilagrow

Great, thanks for your amazing contribution 🤗

shibasisp requested a review from a team as a code owner May 21, 2023 18:05

shibasisp requested review from AlekSi and rumyantseva May 21, 2023 18:05

shibasisp changed the title ~~[#1432] Implement pipeline stage~~ [#1432] Implement $unset pipeline pipeline stage May 21, 2023

mergify bot assigned shibasisp May 21, 2023

shibasisp changed the title ~~[#1432] Implement $unset pipeline pipeline stage~~ [#1432] Implement $unset aggregation pipeline pipeline stage May 21, 2023

shibasisp marked this pull request as draft May 21, 2023 18:15

shibasisp force-pushed the 1432_implement_unset_pipeline branch from a6c9810 to c47c2c4 Compare May 21, 2023 19:14

shibasisp changed the title ~~[#1432] Implement $unset aggregation pipeline pipeline stage~~ Implement $unset aggregation pipeline pipeline stage May 22, 2023

rumyantseva suggested changes May 22, 2023

View reviewed changes

AlekSi added this to the Next milestone May 29, 2023

AlekSi added the code/feature Some user-visible feature is not implemented yet label May 29, 2023

AlekSi mentioned this pull request Jun 1, 2023

Implement simple $addFields/$set aggregation pipeline stages #2590

Closed

mergify bot added the conflict PRs that have merge conflicts label Jun 2, 2023

shibasisp force-pushed the 1432_implement_unset_pipeline branch from 7adb488 to 857d46b Compare June 2, 2023 09:30

mergify bot removed the conflict PRs that have merge conflicts label Jun 2, 2023

shibasisp marked this pull request as ready for review June 2, 2023 09:32

AlekSi requested a review from rumyantseva June 2, 2023 09:35

AlekSi requested changes Jun 2, 2023

View reviewed changes

rumyantseva requested a review from AlekSi June 2, 2023 12:58

rumyantseva changed the title ~~Implement $unset aggregation pipeline pipeline stage~~ Implement $unset aggregation pipeline pipeline stage Jun 2, 2023

rumyantseva changed the title ~~Implement $unset aggregation pipeline pipeline stage~~ Implement $unset aggregation pipeline stage Jun 2, 2023

rumyantseva enabled auto-merge (squash) June 2, 2023 13:03

rumyantseva previously approved these changes Jun 2, 2023

View reviewed changes

AlekSi requested a review from noisersup June 2, 2023 15:44

shibasisp dismissed stale reviews from rumyantseva and AlekSi via 9dac010 June 3, 2023 06:38

rumyantseva requested review from AlekSi and rumyantseva June 5, 2023 08:23

AlekSi modified the milestones: v1.3.0, Next Jun 5, 2023

AlekSi requested a review from chilagrow June 5, 2023 11:14

noisersup previously approved these changes Jun 5, 2023

View reviewed changes

internal/handlers/common/aggregations/stages/unset.go Show resolved Hide resolved

internal/handlers/common/aggregations/stages/unset.go Show resolved Hide resolved

AlekSi enabled auto-merge (squash) June 5, 2023 13:32

rumyantseva reviewed Jun 5, 2023

View reviewed changes

internal/handlers/common/aggregations/stages/unset.go Show resolved Hide resolved

internal/handlers/common/aggregations/stages/unset.go Show resolved Hide resolved

rumyantseva reviewed Jun 5, 2023

View reviewed changes

chilagrow reviewed Jun 6, 2023

View reviewed changes

internal/handlers/common/aggregations/stages/unset.go Outdated Show resolved Hide resolved

auto-merge was automatically disabled June 10, 2023 14:45
Head branch was pushed to by a user without write access

shibasisp dismissed noisersup’s stale review via 0d66ee9 June 10, 2023 14:45

shibasisp added 6 commits June 10, 2023 20:16

[FerretDB#1432] Implement pipeline stage

0ed6df2

fixes lint

bb1ca7a

fixes error handling and integration tests

aa9a9f1

fixes error after rebase

3a49ec5

ignore lint for readability

ba65ad3

Adds more test cases for unset stage

265458f

shibasisp force-pushed the 1432_implement_unset_pipeline branch from 0d66ee9 to 265458f Compare June 10, 2023 15:13

rumyantseva requested review from rumyantseva, noisersup and chilagrow June 13, 2023 08:46

chilagrow approved these changes Jun 14, 2023

View reviewed changes

noisersup approved these changes Jun 14, 2023

View reviewed changes

Merge branch 'main' into 1432_implement_unset_pipeline

fe5cf9d

AlekSi enabled auto-merge (squash) June 14, 2023 08:41

AlekSi assigned noisersup Jun 14, 2023

AlekSi merged commit bdfd6b1 into FerretDB:main Jun 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `$unset` aggregation pipeline stage #2676

Implement `$unset` aggregation pipeline stage #2676

shibasisp commented May 21, 2023 •

edited

Loading

CLAassistant commented May 21, 2023 •

edited

Loading

codecov bot commented May 21, 2023 •

edited

Loading

rumyantseva left a comment

AlekSi commented May 29, 2023

shibasisp commented Jun 1, 2023

mergify bot commented Jun 2, 2023

AlekSi left a comment

rumyantseva left a comment

noisersup left a comment

rumyantseva left a comment

rumyantseva left a comment

chilagrow left a comment

Implement $unset aggregation pipeline stage #2676

Implement $unset aggregation pipeline stage #2676

Conversation

shibasisp commented May 21, 2023 • edited Loading

Description

Readiness checklist

CLAassistant commented May 21, 2023 • edited Loading

codecov bot commented May 21, 2023 • edited Loading

Codecov Report

rumyantseva left a comment

Choose a reason for hiding this comment

AlekSi commented May 29, 2023

shibasisp commented Jun 1, 2023

mergify bot commented Jun 2, 2023

AlekSi left a comment

Choose a reason for hiding this comment

rumyantseva left a comment

Choose a reason for hiding this comment

noisersup left a comment

Choose a reason for hiding this comment

rumyantseva left a comment

Choose a reason for hiding this comment

rumyantseva left a comment

Choose a reason for hiding this comment

chilagrow left a comment

Choose a reason for hiding this comment

Implement `$unset` aggregation pipeline stage #2676

Implement `$unset` aggregation pipeline stage #2676

shibasisp commented May 21, 2023 •

edited

Loading

CLAassistant commented May 21, 2023 •

edited

Loading

codecov bot commented May 21, 2023 •

edited

Loading