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

Support _id aggregation operators for $group stage #3096

Merged
merged 19 commits into from
Jul 26, 2023

Conversation

noisersup
Copy link
Member

@noisersup noisersup commented Jul 20, 2023

Description

Closes #2694.

Readiness checklist

  • I added/updated unit tests (and they pass).
  • I added/updated integration/compatibility tests (and they pass).
  • 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.

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #3096 (afe96e7) into main (19e1cdb) will decrease coverage by 2.19%.
The diff coverage is 80.27%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3096      +/-   ##
==========================================
- Coverage   76.60%   74.41%   -2.19%     
==========================================
  Files         390      390              
  Lines       21330    21419      +89     
==========================================
- Hits        16339    15940     -399     
- Misses       4073     4543     +470     
- Partials      918      936      +18     
Files Changed Coverage Δ
internal/handlers/commonerrors/error.go 80.00% <ø> (ø)
internal/handlers/commonerrors/errorcode_string.go 80.00% <ø> (ø)
...ernal/handlers/common/aggregations/stages/group.go 84.27% <80.27%> (-4.41%) ⬇️

... and 27 files with indirect coverage changes

Flag Coverage Δ
hana ?
integration 70.84% <80.27%> (-2.28%) ⬇️
mongodb 5.51% <0.00%> (-0.03%) ⬇️
pg 61.16% <80.27%> (-5.36%) ⬇️
shard-1 55.88% <12.24%> (-0.28%) ⬇️
shard-2 35.81% <0.00%> (-18.36%) ⬇️
shard-3 59.24% <80.27%> (+0.18%) ⬆️
sqlite 46.37% <0.00%> (-0.25%) ⬇️
unit 24.11% <0.00%> (-0.18%) ⬇️

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

@noisersup noisersup force-pushed the aggregation-group-id-operators-2964 branch from 34bdbbd to 5d30c49 Compare July 21, 2023 11:44
@noisersup noisersup added the code/chore Code maintenance improvements label Jul 21, 2023
@noisersup noisersup marked this pull request as ready for review July 21, 2023 13:07
@noisersup noisersup requested a review from a team as a code owner July 21, 2023 13:07
@noisersup noisersup requested review from AlekSi and rumyantseva July 21, 2023 13:07
@noisersup noisersup enabled auto-merge (squash) July 21, 2023 13:07
@noisersup noisersup requested review from a team, quasilyte and chilagrow July 21, 2023 13:07
@rumyantseva
Copy link
Contributor

@noisersup the linter fails!

Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

See comments about test coverage. Also there are failing tests on CI.

internal/handlers/common/aggregations/stages/group.go Outdated Show resolved Hide resolved
internal/handlers/common/aggregations/stages/group.go Outdated Show resolved Hide resolved
internal/handlers/common/aggregations/stages/group.go Outdated Show resolved Hide resolved
@AlekSi AlekSi removed their request for review July 24, 2023 06:49
@AlekSi AlekSi added this to the Next milestone Jul 24, 2023
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Could you check the test coverage again please 🙏 . There are still quite a bit that are not covered.

The error handing code seems quite duplicated, do you think we can merge them as stage input error handling?

Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Amazing work! 🚀

@noisersup noisersup merged commit 777222a into FerretDB:main Jul 26, 2023
@noisersup noisersup deleted the aggregation-group-id-operators-2964 branch July 26, 2023 01:42
yonarw pushed a commit to yonarw/FerretDB that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/chore Code maintenance improvements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support recursive aggregation operators
4 participants