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.

Sign up for GitHub

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

Jump to bottom

Use cursors with iterator in aggregate command #2929

Merged
merged 19 commits into from
Jun 30, 2023
Merged

Use cursors with iterator in aggregate command #2929

merged 19 commits into from
Jun 30, 2023

Conversation

chilagrow
Copy link
Member

@chilagrow chilagrow commented Jun 28, 2023 •
edited
Loading

Description

Closes #2877.
Closes #1733.

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.

@chilagrow chilagrow added the code/chore Code maintenance improvements label Jun 28, 2023
@chilagrow chilagrow self-assigned this Jun 28, 2023
@codecov
Copy link

codecov bot commented Jun 28, 2023 •
edited
Loading

Codecov Report

Merging #2929 (7aa702b) into main (5f0ddcb) will increase coverage by 13.96%.
The diff coverage is 98.43%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2929       +/-   ##
===========================================
+ Coverage   50.26%   64.23%   +13.96%     
===========================================
  Files         451      451               
  Lines       23914    23951       +37     
===========================================
+ Hits        12021    15385     +3364     
+ Misses      10879     7605     -3274     
+ Partials     1014      961       -53
Impacted Files Coverage Δ
internal/handlers/pg/msg_aggregate.go 86.71% <97.82%> (+9.89%) ⬆️
integration/helpers.go 83.08% <100.00%> (+10.96%) ⬆️
...l/handlers/common/aggregations/stages/collstats.go 85.48% <100.00%> (+0.73%) ⬆️
...ernal/handlers/common/aggregations/stages/group.go 88.81% <100.00%> (+9.70%) ⬆️
...rnal/handlers/common/aggregations/stages/unwind.go 83.49% <100.00%> (+0.49%) ⬆️

... and 84 files with indirect coverage changes

Flag Coverage Δ
integration 57.67% <98.43%> (+14.87%) ⬆️
mongodb 4.86% <9.37%> (+0.03%) ⬆️
pg 57.60% <98.43%> (+15.21%) ⬆️
shard-1 42.15% <71.87%> (+37.33%) ⬆️
shard-2 43.10% <73.43%> (?)
shard-3 46.05% <84.37%> (+3.66%) ⬆️
unit 24.24% <0.00%> (-0.04%) ⬇️

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

@chilagrow chilagrow changed the title Use iterator for aggregation process stages output Use iterator for aggregation output Jun 29, 2023
@chilagrow chilagrow marked this pull request as ready for review June 29, 2023 03:56
@chilagrow chilagrow requested a review from a team as a code owner June 29, 2023 03:56
@chilagrow chilagrow requested review from AlekSi, w84thesun, a team, rumyantseva and noisersup June 29, 2023 03:56
@chilagrow chilagrow enabled auto-merge (squash) June 29, 2023 03:56
@chilagrow chilagrow changed the title Use iterator for aggregation output Use iterator for aggregation cursor Jun 29, 2023
@chilagrow chilagrow changed the title Use iterator for aggregation cursor Use iterator for aggregation cursor response Jun 29, 2023
@chilagrow chilagrow changed the title Use iterator for aggregation cursor response Use cursors with iterator in aggregation command Jun 29, 2023
@chilagrow chilagrow changed the title Use cursors with iterator in aggregation command Use cursors with iterator in aggregate command Jun 29, 2023
@AlekSi AlekSi disabled auto-merge June 29, 2023 07:25
@AlekSi AlekSi enabled auto-merge (squash) June 29, 2023 07:25
@AlekSi AlekSi disabled auto-merge June 29, 2023 07:26
@AlekSi AlekSi enabled auto-merge (squash) June 29, 2023 07:26
AlekSi
AlekSi reviewed Jun 29, 2023
View reviewed changes
Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

LGTM overall, but I wonder if we have tests for a combination of aggregate+getMore and, if not, if we could add them without copy&pasting find+getMore tests

integration/aggregate_documents_compat_test.go Outdated Show resolved Hide resolved
integration/aggregate_documents_compat_test.go Outdated Show resolved Hide resolved
internal/handlers/pg/msg_aggregate.go Outdated Show resolved Hide resolved
internal/handlers/pg/msg_aggregate.go Show resolved Hide resolved
internal/handlers/pg/msg_aggregate.go Outdated Show resolved Hide resolved
internal/handlers/pg/msg_aggregate.go Outdated Show resolved Hide resolved
@chilagrow
Copy link
Member Author

LGTM overall, but I wonder if we have tests for a combination of aggregate+getMore and, if not, if we could add them without copy&pasting find+getMore tests

@AlekSi, currently combination tests for aggregate and getMore tests batchSizes, but it's implicit because getMore is called via cursor https://github.com/FerretDB/FerretDB/blob/main/integration/aggregate_documents_test.go#L821 Do we think we need to expand?

getMore tests are mainly done with find at the moment like this one https://github.com/FerretDB/FerretDB/blob/main/integration/query_test.go#L959

@chilagrow chilagrow requested a review from AlekSi June 29, 2023 10:42
w84thesun
w84thesun approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

👍

internal/handlers/pg/msg_aggregate.go Show resolved Hide resolved
noisersup
noisersup approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@noisersup noisersup left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me

@AlekSi AlekSi added this to the Next milestone Jun 30, 2023
@AlekSi AlekSi merged commit a5cc636 into FerretDB:main Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@w84thesun w84thesun w84thesun approved these changes

@noisersup noisersup noisersup approved these changes

@AlekSi AlekSi AlekSi approved these changes

@rumyantseva rumyantseva Awaiting requested review from rumyantseva rumyantseva was automatically assigned from FerretDB/core

Assignees

@chilagrow chilagrow

Labels
code/chore Code maintenance improvements
Projects
Current Engineering
Archived in project
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.

Do not consume all documents in processStagesDocuments Support getMore command
4 participants
@chilagrow @AlekSi @w84thesun @noisersup