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

Implement positional operator in projection #2688

Merged
merged 22 commits into from
May 26, 2023
Merged

Implement positional operator in projection #2688

merged 22 commits into from
May 26, 2023

Conversation

chilagrow
Copy link
Member

@chilagrow chilagrow commented May 23, 2023 •
edited
Loading

Description

Closes #1709.

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/feature Some user-visible feature is not implemented yet label May 23, 2023
@chilagrow chilagrow self-assigned this May 23, 2023
@codecov
Copy link

codecov bot commented May 23, 2023 •
edited
Loading

Codecov Report

Merging #2688 (dcdb827) into main (d0022fb) will increase coverage by 0.36%.
The diff coverage is 92.74%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2688      +/-   ##
==========================================
+ Coverage   62.67%   63.04%   +0.36%     
==========================================
  Files         435      436       +1     
  Lines       22378    22583     +205     
==========================================
+ Hits        14026    14237     +211     
+ Misses       7425     7421       -4     
+ Partials      927      925       -2
Impacted Files Coverage Δ
internal/handlers/commonerrors/error.go 80.00% <ø> (ø)
internal/handlers/commonerrors/errorcode_string.go 80.00% <ø> (ø)
internal/handlers/sqlite/msg_find.go 0.00% <0.00%> (ø)
internal/handlers/tigris/msg_find.go 0.00% <0.00%> (ø)
internal/handlers/common/positional_operator.go 79.45% <79.45%> (ø)
internal/handlers/common/projection.go 88.52% <99.13%> (+7.93%) ⬆️
...ommon/aggregations/stages/projection/projection.go 85.07% <100.00%> (+2.87%) ⬆️
internal/handlers/common/projection_iterator.go 100.00% <100.00%> (+11.11%) ⬆️
internal/handlers/pg/msg_find.go 71.96% <100.00%> (ø)
Flag Coverage Δ
integration 56.49% <92.74%> (+0.42%) ⬆️
mongodb 4.97% <0.00%> (-0.05%) ⬇️
pg 56.41% <92.74%> (+0.43%) ⬆️
unit 24.68% <0.00%> (-0.29%) ⬇️

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

@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

@chilagrow this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label May 24, 2023
@mergify mergify bot removed the conflict PRs that have merge conflicts label May 24, 2023
@chilagrow chilagrow requested review from a team, w84thesun, rumyantseva and noisersup May 24, 2023 08:28
@chilagrow chilagrow marked this pull request as ready for review May 24, 2023 08:28
@chilagrow chilagrow requested review from AlekSi, ptrfarkas and a team as code owners May 24, 2023 08:28
@chilagrow chilagrow enabled auto-merge (squash) May 24, 2023 08:28
noisersup
noisersup reviewed May 24, 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.

Not a full review yet, small bug for now. It also looks to me that aggregation projection starts to being very huge :)

internal/handlers/common/aggregations/projection.go Outdated Show resolved Hide resolved
@noisersup noisersup self-requested a review May 24, 2023 10:40
rumyantseva
rumyantseva reviewed May 24, 2023
View reviewed changes
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.

Great job with tackling such a complex task and making significant progress!

I asked a few questions about the places where I'm not sure. But it seems that I need some help to understand projection overall, so I will schedule a pair programming session to learn more things about projection ;)

internal/handlers/common/aggregations/projection.go Outdated Show resolved Hide resolved
internal/handlers/common/positional_operator.go Outdated Show resolved Hide resolved
internal/handlers/common/positional_operator.go Outdated Show resolved Hide resolved
integration/query_projection_compat_test.go Show resolved Hide resolved
internal/handlers/common/positional_operator.go Show resolved Hide resolved
internal/handlers/common/positional_operator.go Outdated Show resolved Hide resolved
internal/handlers/common/projection_iterator.go Show resolved Hide resolved
internal/handlers/common/positional_operator.go Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

@chilagrow this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label May 24, 2023
noisersup
noisersup reviewed May 24, 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.

Apart from my previous comment it looks good to me! It would be still great to have in mind some refactoring in projections parts (maybe some issue).

w84thesun
w84thesun reviewed May 24, 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.

Good job! Added a couple of comments.

integration/query_projection_compat_test.go Show resolved Hide resolved
internal/handlers/common/projection.go Outdated Show resolved Hide resolved
rumyantseva
rumyantseva previously approved these changes May 25, 2023
View reviewed changes
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.

I tried some things locally, and I think the PR solves the task.
The code of projection related things seems to be too complicate (not only positional operator, but overall), but I don't have an idea how to make it simpler. So I suggest to merge it as is and if we have a better understanding of error sequence or other things, we can always improve the approach.

@mergify mergify bot removed the conflict PRs that have merge conflicts label May 26, 2023
@chilagrow chilagrow requested a review from rumyantseva May 26, 2023 07:36
rumyantseva
rumyantseva approved these changes May 26, 2023
View reviewed changes
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

noisersup
noisersup approved these changes May 26, 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.

Everything seems great! Thanks for a lot of test coverage

AlekSi
AlekSi approved these changes May 26, 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 for docs. I did not review the code

@chilagrow chilagrow merged commit 94537f3 into FerretDB:main May 26, 2023
@AlekSi AlekSi added this to the v1.3.0 milestone Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noisersup noisersup noisersup approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@AlekSi AlekSi AlekSi approved these changes

@ptrfarkas ptrfarkas Awaiting requested review from ptrfarkas ptrfarkas is a code owner

@w84thesun w84thesun Awaiting requested review from w84thesun

Assignees

@chilagrow chilagrow

Labels
code/feature Some user-visible feature is not implemented yet
Projects
Current Engineering
Archived in project
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Implement projection positional $ operator
5 participants
@chilagrow @AlekSi @w84thesun @rumyantseva @noisersup