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

Separate codebase for aggregation $project and query projection #2631

Merged
merged 5 commits into from
May 15, 2023

Conversation

chilagrow
Copy link
Contributor

@chilagrow chilagrow commented May 15, 2023

Description

Closes #2430.

Projection in aggregation and query will diverge. They work differently with operators. In this PR, we create a separate codebase for aggregation.

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 May 15, 2023
@chilagrow chilagrow self-assigned this May 15, 2023
@chilagrow chilagrow changed the title Use separate codebase for aggregation $project and query projection Separate codebase for aggregation $project and query projection May 15, 2023
@chilagrow chilagrow marked this pull request as ready for review May 15, 2023 01:04
@chilagrow chilagrow requested a review from a team as a code owner May 15, 2023 01:04
@chilagrow chilagrow requested review from w84thesun, rumyantseva, a team and noisersup May 15, 2023 01:04
@chilagrow chilagrow enabled auto-merge (squash) May 15, 2023 01:04
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #2631 (edb9ce3) into main (89c2880) will decrease coverage by 0.50%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2631      +/-   ##
==========================================
- Coverage   28.74%   28.25%   -0.50%     
==========================================
  Files         419      421       +2     
  Lines       20480    20837     +357     
==========================================
  Hits         5887     5887              
- Misses      13995    14352     +357     
  Partials      598      598              
Impacted Files Coverage Δ
...nternal/handlers/common/aggregations/projection.go 0.00% <0.00%> (ø)
...andlers/common/aggregations/projection_iterator.go 0.00% <0.00%> (ø)
...nal/handlers/common/aggregations/stages/project.go 0.00% <0.00%> (ø)
internal/handlers/common/projection.go 0.00% <ø> (ø)
Flag Coverage Δ
integration 5.32% <0.00%> (-0.10%) ⬇️
mongodb 5.32% <0.00%> (-0.10%) ⬇️
unit 26.73% <0.00%> (-0.50%) ⬇️

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

w84thesun
w84thesun previously approved these changes May 15, 2023
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.

LGTM!

Copy link
Member

@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.

As we agreed, I approve because this code doesn't make things worse - we continue working the same way, just getting ready to handle aggregation separately. Also, now we have a separate issue to continue testing aggregation.

@chilagrow chilagrow merged commit 93a6eb6 into FerretDB:main May 15, 2023
@AlekSi AlekSi added this to the Next milestone May 16, 2023
// - `ErrProjectionExIn` when there is exclusion in inclusion projection;
// - `ErrProjectionInEx` when there is inclusion in exclusion projection;
// - `ErrNotImplemented` when there is unimplemented projection operators and expressions;
// - `errProjectionEmpty` when projection document is empty.
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ I don't think it is correct for the same exported function to return both protocol-error errors like CommandError with ErrProjectionExIn code and internal errors like errProjectionEmpty. Not to mention that documentation mixes protocol error codes and Go-level error values.

return nil, false, lazyerrors.Error(err)
}

if strings.Contains(key, "$") {
Copy link
Member

Choose a reason for hiding this comment

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

I think that should be strings.HasPrefix

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 dot notation in query projections
4 participants