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 one implementation for finding path values #3087

Merged
merged 58 commits into from
Aug 4, 2023
Merged

Use one implementation for finding path values #3087

merged 58 commits into from
Aug 4, 2023

Conversation

chilagrow
Copy link
Member

@chilagrow chilagrow commented Jul 20, 2023 •
edited
Loading

Description

Closes #2348.

I created a new package commonpath.

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.

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

codecov bot commented Jul 20, 2023 •
edited
Loading

Codecov Report

Merging #3087 (307792e) into main (60ae858) will decrease coverage by 0.54%.
The diff coverage is 88.11%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3087      +/-   ##
==========================================
- Coverage   75.62%   75.08%   -0.54%     
==========================================
  Files         393      395       +2     
  Lines       21968    21953      -15     
==========================================
- Hits        16613    16484     -129     
- Misses       4390     4510     +120     
+ Partials      965      959       -6
Files Changed Coverage Δ
integration/compattestcaseresulttype_string.go 0.00% <ø> (ø)
integration/helpers.go 87.23% <ø> (+0.35%) ⬆️
internal/backends/error.go 54.28% <ø> (+2.85%) ⬆️
internal/backends/errorcode_string.go 50.00% <ø> (ø)
...ommon/aggregations/stages/projection/projection.go 86.97% <ø> (ø)
internal/handlers/common/sort.go 91.42% <ø> (ø)
internal/types/array.go 85.32% <ø> (ø)
internal/types/document.go 90.99% <ø> (ø)
internal/types/patherrorcode_string.go 0.00% <0.00%> (ø)
internal/handlers/pg/pgdb/query.go 81.96% <33.33%> (-0.66%) ⬇️
... and 18 more

... and 17 files with indirect coverage changes

Flag Coverage Δ
hana ?
integration 71.40% <86.36%> (-0.63%) ⬇️
mongodb 5.36% <3.14%> (-0.01%) ⬇️
pg 65.05% <86.01%> (-0.16%) ⬇️
shard-1 56.29% <77.97%> (+0.98%) ⬆️
shard-2 52.60% <75.52%> (-0.51%) ⬇️
shard-3 55.52% <76.92%> (-2.87%) ⬇️
sqlite 33.61% <44.75%> (-4.82%) ⬇️
unit 24.97% <41.60%> (+0.27%) ⬆️

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

@chilagrow chilagrow changed the title Merge getting nested path implementation to one Use one implementation for finding values for a path Jul 20, 2023
@chilagrow chilagrow changed the title Use one implementation for finding values for a path Use one implementation for finding path values Jul 20, 2023
@chilagrow chilagrow marked this pull request as ready for review July 20, 2023 08:25
@chilagrow chilagrow requested a review from a team as a code owner July 20, 2023 08:25
@chilagrow chilagrow requested review from AlekSi, rumyantseva, a team, quasilyte and noisersup July 20, 2023 08:25
@chilagrow chilagrow enabled auto-merge (squash) July 20, 2023 08:25
@AlekSi AlekSi added the do not merge PRs that should not be merged label Jul 20, 2023
@AlekSi
Copy link
Member

AlekSi commented Jul 20, 2023

(please review, but don't merge yet)

rumyantseva
rumyantseva reviewed Jul 20, 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.

Based on my past experience with path-related code, the implementation looks clear to me. Asked a couple of questions where I'm not sure.

internal/handlers/commonpath/path.go Outdated Show resolved Hide resolved
internal/handlers/commonpath/path.go Outdated Show resolved Hide resolved
internal/handlers/commonpath/path.go Outdated Show resolved Hide resolved
internal/handlers/commonpath/path_test.go Outdated Show resolved Hide resolved
noisersup
noisersup previously approved these changes Jul 20, 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.

Overall it seems that it handles existing logic and it's great that we merge all of those functions into the single one with options

internal/handlers/common/distinct.go Outdated Show resolved Hide resolved
@AlekSi AlekSi modified the milestones: v1.7.0, Next Jul 31, 2023
noisersup
noisersup previously approved these changes Jul 31, 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.

LGTM

@chilagrow chilagrow dismissed stale reviews from noisersup and rumyantseva via 9c4899c August 1, 2023 08:24
@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2023

@chilagrow this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Aug 3, 2023
AlekSi
AlekSi previously approved these changes Aug 3, 2023
View reviewed changes
@mergify mergify bot removed the conflict PRs that have merge conflicts label Aug 4, 2023
rumyantseva
rumyantseva approved these changes Aug 4, 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.

In addition to the changes, I also like the documentation and examples (e.g. $unwind). It brings more clarity about how things work and why!

@chilagrow chilagrow merged commit 658b8e2 into FerretDB:main Aug 4, 2023
yonarw pushed a commit to yonarw/FerretDB that referenced this pull request Aug 31, 2023
@chilagrow @yonarw
Use one implementation for finding path values (FerretDB#3087)
566805e 
Closes FerretDB#2348.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rumyantseva rumyantseva rumyantseva approved these changes

@AlekSi AlekSi AlekSi approved these changes

@noisersup noisersup Awaiting requested review from noisersup

Assignees

@chilagrow chilagrow

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

Successfully merging this pull request may close these issues.

Merge aggregation.getPathValue() and common.getDocumentsAtSuffix()
4 participants
@chilagrow @AlekSi @rumyantseva @noisersup