-
Notifications
You must be signed in to change notification settings - Fork 409
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
Separate codebase for aggregation $project
and query projection
#2631
Conversation
$project
and query projection
$project
and query projection
Codecov Report
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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.
// - `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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, "$") { |
There was a problem hiding this comment.
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
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
task all
, and it passed.@FerretDB/core
), Labels, Project and project's Sprint fields.