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

Fix index creation on nested fields #2637

Merged
merged 11 commits into from
May 19, 2023
Merged

Fix index creation on nested fields #2637

merged 11 commits into from
May 19, 2023

Conversation

wqhhust
Copy link
Contributor

@wqhhust wqhhust commented May 15, 2023 •
edited by rumyantseva
Loading

Description

Closes #2606.

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.

@wqhhust wqhhust requested a review from a team as a code owner May 15, 2023 13:56
@wqhhust wqhhust requested review from AlekSi and chilagrow May 15, 2023 13:56
@wqhhust wqhhust mentioned this pull request May 15, 2023
Closed
@codecov
Copy link

codecov bot commented May 15, 2023 •
edited
Loading

Codecov Report

Merging #2637 (d22147f) into main (bd5274f) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2637      +/-   ##
==========================================
- Coverage   26.96%   26.95%   -0.02%     
==========================================
  Files         432      432              
  Lines       21958    21965       +7     
==========================================
- Hits         5922     5920       -2     
- Misses      15431    15441      +10     
+ Partials      605      604       -1
Impacted Files Coverage Δ
internal/handlers/pg/pgdb/indexes.go 63.30% <100.00%> (+1.94%) ⬆️

... and 2 files with indirect coverage changes

Flag Coverage Δ
integration 5.11% <0.00%> (-0.01%) ⬇️
mongodb 5.11% <0.00%> (-0.01%) ⬇️
unit 25.39% <100.00%> (-0.02%) ⬇️

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

@AlekSi AlekSi added this to the Next milestone May 15, 2023
@AlekSi AlekSi requested review from rumyantseva and removed request for chilagrow May 16, 2023 04:52
@AlekSi AlekSi added the code/bug Some user-visible feature works incorrectly label May 16, 2023
rumyantseva
rumyantseva reviewed May 16, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The change itself looks reasonable!
We need to write tests to ensure that everything works correctly.

We have tests for indexes in internal/handlers/pg/pgdb/indexes_test.go. One option would be to extend TestCreateIndexIfNotExists test to cover nested fields too. A similar approach to the current one - create an index for a nested field and compare created index definition with expected one.

And also we can add one test case in TestDropIndexes to ensure that it's possible to drop indexes for nested fields.

Do you any help with it?

@wqhhust wqhhust requested a review from rumyantseva May 16, 2023 14:55
@wqhhust
Copy link
Contributor Author

wqhhust commented May 16, 2023

added the test

rumyantseva
rumyantseva reviewed May 16, 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 defining test cases!

Let's introduce table tests for TestCreateIndexIfNotExists.
Also, could you change the title of the PR, please? The title should start with a verb, shouldn't include the issue number, and should match the change that is made.

internal/handlers/pg/pgdb/indexes_test.go Outdated Show resolved Hide resolved
@wqhhust wqhhust changed the title 2606 index nested fields fix Indexes are created incorrectly for nested fields (PR 2606) May 17, 2023
@wqhhust wqhhust changed the title fix Indexes are created incorrectly for nested fields (PR 2606) Fix Indexes are created incorrectly for nested fields May 17, 2023
@wqhhust wqhhust changed the title Fix Indexes are created incorrectly for nested fields Fix Index creation on nested fields May 17, 2023
@wqhhust
Copy link
Contributor Author

wqhhust commented May 17, 2023

fixed.
image

@wqhhust wqhhust requested a review from rumyantseva May 17, 2023 02:45
@rumyantseva rumyantseva enabled auto-merge (squash) May 17, 2023 08:18
@rumyantseva rumyantseva changed the title Fix Index creation on nested fields Fix index creation on nested fields May 17, 2023
rumyantseva
rumyantseva reviewed May 17, 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, now we have table tests!

I left a couple of relatively minor comments, but overall looks good to me!

internal/handlers/pg/pgdb/indexes_test.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/indexes_test.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/indexes.go Outdated Show resolved Hide resolved
auto-merge was automatically disabled May 17, 2023 21:05

Head branch was pushed to by a user without write access

@wqhhust wqhhust requested a review from rumyantseva May 17, 2023 21:12
chilagrow
chilagrow reviewed May 18, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Great work, just one tiny comment!

internal/handlers/pg/pgdb/indexes.go Outdated Show resolved Hide resolved
@wqhhust wqhhust requested a review from chilagrow May 19, 2023 06:37
chilagrow
chilagrow approved these changes May 19, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the contribution.

@rumyantseva rumyantseva enabled auto-merge (squash) May 19, 2023 08:17
rumyantseva
rumyantseva approved these changes May 19, 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

@rumyantseva rumyantseva merged commit cc02a65 into FerretDB:main May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chilagrow chilagrow chilagrow approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@AlekSi AlekSi Awaiting requested review from AlekSi AlekSi is a code owner automatically assigned from FerretDB/leads

Assignees

@wqhhust wqhhust

Labels
code/bug Some user-visible feature works incorrectly
Projects
Current Engineering
Archived in project
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

Indexes are created incorrectly for nested fields
4 participants
@wqhhust @rumyantseva @chilagrow @AlekSi