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 createIndexes for unique indexes #2814

Merged
merged 51 commits into from
Jun 16, 2023
Merged

Implement createIndexes for unique indexes #2814

merged 51 commits into from
Jun 16, 2023

Conversation

w84thesun
Copy link
Contributor

@w84thesun w84thesun commented Jun 7, 2023 •
edited
Loading

Description

Closes #2045.

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.

@w84thesun w84thesun added the code/feature Some user-visible feature is not implemented yet label Jun 7, 2023
@w84thesun w84thesun self-assigned this Jun 7, 2023
@w84thesun w84thesun added the not ready Issues that are not ready to be worked on; PRs that should skip CI label Jun 7, 2023
@codecov
Copy link

codecov bot commented Jun 7, 2023 •
edited
Loading

Codecov Report

Merging #2814 (2650491) into main (846c0b9) will increase coverage by 0.04%.
The diff coverage is 80.64%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2814      +/-   ##
==========================================
+ Coverage   63.73%   63.77%   +0.04%     
==========================================
  Files         448      448              
  Lines       22818    22890      +72     
==========================================
+ Hits        14542    14598      +56     
- Misses       7316     7334      +18     
+ Partials      960      958       -2
Impacted Files Coverage Δ
internal/handlers/commonerrors/error.go 80.00% <ø> (ø)
internal/handlers/commonerrors/errorcode_string.go 80.00% <ø> (ø)
internal/handlers/tigris/msg_insert.go 0.00% <0.00%> (ø)
internal/handlers/pg/pgdb/indexes.go 75.00% <50.00%> (-1.98%) ⬇️
internal/handlers/pg/msg_createindexes.go 77.55% <79.41%> (+0.77%) ⬆️
integration/helpers.go 82.89% <100.00%> (-1.63%) ⬇️
internal/handlers/pg/msg_insert.go 83.65% <100.00%> (-0.46%) ⬇️
internal/handlers/pg/msg_listindexes.go 72.00% <100.00%> (+2.00%) ⬆️
internal/handlers/pg/pgdb/collections.go 70.00% <100.00%> (ø)
internal/handlers/pg/pgdb/database_metadata.go 80.69% <100.00%> (+0.51%) ⬆️
Flag Coverage Δ
integration 56.84% <80.64%> (+0.02%) ⬆️
mongodb 4.26% <0.00%> (-0.02%) ⬇️
pg 56.75% <80.64%> (+0.02%) ⬆️
shard-1 43.53% <62.36%> (+0.29%) ⬆️
shard-2 41.22% <30.10%> (+0.35%) ⬆️
shard-3 42.55% <72.04%> (-1.15%) ⬇️
unit 24.82% <27.63%> (+0.01%) ⬆️

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

@mergify
Copy link
Contributor

mergify bot commented Jun 9, 2023

@w84thesun this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Jun 9, 2023
Dmitry added 2 commits June 9, 2023 15:56
Merge branch 'main' into issue-2045-create-indexes
dc33149 
# Conflicts:
#	integration/indexes_compat_test.go
Merge branch 'main' into issue-2045-create-indexes
189f139 
# Conflicts:
#	integration/indexes_compat_test.go
@mergify mergify bot removed the conflict PRs that have merge conflicts label Jun 9, 2023
@w84thesun w84thesun removed the not ready Issues that are not ready to be worked on; PRs that should skip CI label Jun 9, 2023
@w84thesun w84thesun changed the title Implement createIndexes for unique indexes Implement createIndexes for unique indexes Jun 9, 2023
@w84thesun w84thesun requested a review from chilagrow June 15, 2023 15:04
noisersup
noisersup previously approved these changes Jun 15, 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.

Looks great!

@chilagrow
Copy link
Member

chilagrow commented Jun 16, 2023 •
edited
Loading

Is this case handled? The data is from task env-data and running it on task run-proxy. If not maybe this is something we might need to handle in other issue.

test> db.ArrayInt32s.createIndex({v: 1}, {unique: false})
v_1
test> db.ArrayInt32s.createIndex({v: 1}, {unique: true})
MongoServerError: An existing index has the same name as the requested index. When index names are not specified, they are auto generated and can cause conflicts. Please refer to our documentation. Requested index: { v: 2, unique: true, key: { v: 1 }, name: "v_1" }, existing index: { v: 2, key: { v: 1 }, name: "v_1" }

chilagrow
chilagrow reviewed Jun 16, 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.

So many edge cases with unique index creation 😮

My main comments are just testing error messages, others are not so important.

integration/indexes_compat_test.go Show resolved Hide resolved
internal/handlers/pg/msg_createindexes.go Show resolved Hide resolved
integration/insert_compat_test.go Show resolved Hide resolved
integration/indexes_compat_test.go Show resolved Hide resolved
internal/handlers/pg/msg_insert.go Show resolved Hide resolved
internal/handlers/pg/pgdb/indexes.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/database_metadata.go Outdated Show resolved Hide resolved
@w84thesun w84thesun requested review from noisersup and chilagrow June 16, 2023 02:58
chilagrow
chilagrow previously approved these changes Jun 16, 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!

internal/handlers/pg/msg_createindexes.go Show resolved Hide resolved
integration/indexes_compat_test.go Show resolved Hide resolved
integration/indexes_compat_test.go Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Jun 16, 2023

@w84thesun this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Jun 16, 2023
noisersup
noisersup previously approved these changes Jun 16, 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 (There are some merge conflicts to resolve)

@w84thesun w84thesun dismissed stale reviews from noisersup and chilagrow via 2650491 June 16, 2023 12:58
@w84thesun w84thesun requested review from noisersup and chilagrow June 16, 2023 12:58
@mergify mergify bot removed the conflict PRs that have merge conflicts label Jun 16, 2023
rumyantseva
rumyantseva approved these changes Jun 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.

LGTM overall. I have a couple of minor points, but I suggest we leave them for now to get the task done.

internal/handlers/pg/pgdb/indexes.go Show resolved Hide resolved
internal/handlers/pg/pgdb/database_metadata.go Show resolved Hide resolved
noisersup
noisersup approved these changes Jun 16, 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

@w84thesun w84thesun merged commit d38ab72 into FerretDB:main Jun 16, 2023
@w84thesun w84thesun deleted the issue-2045-create-indexes branch June 16, 2023 20:07
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 Awaiting requested review from AlekSi AlekSi is a code owner automatically assigned from FerretDB/leads

@chilagrow chilagrow Awaiting requested review from chilagrow

Assignees

@w84thesun w84thesun

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

Successfully merging this pull request may close these issues.

Implement createIndexes for unique indexes
4 participants
@w84thesun @chilagrow @rumyantseva @noisersup