feat: Non-unique secondary index #1450

islamaliev · 2023-05-04T16:17:34Z

Relevant issue(s)

Resolves #297

Description

This PR introduces first functionality for secondary indexes which includes:

specifying index for a collection as part of the collection schema and parsing it
mocks for interfaces the index functionality depends on
db methods to create, delete and retrieve indexes
storing index data in the system storage
storing indexed fields' values to data storage
change to the testing framework to allow testing of indexes

This PR doesn't include using indexes for fetching data for queries to keep the PR smaller.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Unit tests

Specify the platform(s) on which this was tested:

MacOS

codecov · 2023-05-04T16:17:49Z

Codecov Report

Patch coverage: 97.23% and project coverage change: +1.16 🎉

Comparison is base (da7b3f9) 72.26% compared to head (3919472) 73.42%.

@@             Coverage Diff             @@
##           develop    #1450      +/-   ##
===========================================
+ Coverage    72.26%   73.42%   +1.16%     
===========================================
  Files          185      187       +2     
  Lines        18478    19291     +813     
===========================================
+ Hits         13353    14164     +811     
+ Misses        4076     4075       -1     
- Partials      1049     1052       +3

Flag	Coverage Δ
all-tests	`73.42% <97.23%> (+1.16%)`	⬆️

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

Impacted Files	Coverage Δ
request/graphql/schema/types/types.go	`100.00% <ø> (ø)`
db/collection.go	`70.99% <89.47%> (+2.20%)`	⬆️
request/graphql/schema/collection.go	`91.80% <96.36%> (+4.86%)`	⬆️
db/errors.go	`87.58% <96.55%> (+9.69%)`	⬆️
db/index.go	`97.60% <97.60%> (ø)`
db/collection_index.go	`98.31% <98.31%> (ø)`
client/descriptions.go	`95.24% <100.00%> (+2.38%)`	⬆️
core/key.go	`89.67% <100.00%> (+3.59%)`	⬆️
db/collection_get.go	`81.25% <100.00%> (+31.25%)`	⬆️
db/fetcher/encoded_doc.go	`62.41% <100.00%> (+0.54%)`	⬆️
... and 5 more

... and 8 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff81968...3919472. Read the comment docs.

db/errors.go

db/index.go

client/index.go

core/key.go

client/collection.go

core/key.go

AndrewSisley

Submitting a partial review for now, as I need a breather from it - it looks good in general, but I have have few low-scope todos so far

db/fetcher/encoded_doc.go

.github/codecov.yml

client/collection.go

client/descriptions.go

core/key.go

AndrewSisley

Taking another break - added some more comments :)

db/collection_get.go

db/collection_index.go

db/index.go

db/collection_index.go

db/index.go

request/graphql/schema/collection.go

fredcarle

Overall I think you're on a good path. I've got a few comments to start. I haven't looked at the tests extensively yet but on first pass I like the mocks.

core/key.go

db/fetcher/encoded_doc.go

db/fetcher/fetcher.go

db/index.go

request/graphql/parser.go

db/collection.go

AndrewSisley

Made it all the way through - looks good, but I think you really need to add some integration tests, and there are a few localised issues that need addressing.

db/index.go

request/graphql/schema/collection.go

db/collection_index.go

tests/integration/schema/updates/index/simple_test.go

AndrewSisley

Gave the integration tests a look over, will go over the prod code again once you re-quest review. Tests look great, thanks a bunch for adding them now, just left a couple of small comments.

tests/integration/index/create_test.go

tests/integration/index/drop_test.go

tests/integration/index/create_test.go

tests/integration/index/drop_test.go

tests/integration/index/create_test.go

datastore/mocks/utils.go

datastore/mocks/DAGStore.go

client/index.go

request/graphql/schema/index_test.go

db/index_test.go

db/indexed_docs_test.go

db/index.go

db/fetcher/mocks/utils.go

db/collection_index.go

request/graphql/parser/request.go

core/key.go

Rely on CBOR's nil-value distinction instead of manual

Correctly implement Txn

AndrewSisley

Really great work Islam, looks good to me :)

fredcarle

Just a few more todos before I approve. I noted a few methods that should have documentation as they are exported. I think there are a few others in your PR that are exported without documentation. I'm flagging them as this is good for internal devs when looking at the code but also because it adds useful information with intelisense in IDEs.

db/collection_index.go

fredcarle

LGTM! Great work Islam!

addressed all comments

Resolves sourcenetwork#297 ## Description This change introduces first functionality for secondary indexes which includes: - specifying index for a collection as part of the collection schema and parsing it - mocks for interfaces the index functionality depends on - db methods to create, delete and retrieve indexes - storing index data in the system storage - storing indexed fields' values to data storage - change to the testing framework to allow testing of indexes This change doesn't include using indexes for fetching data for queries to keep the PR smaller.

islamaliev self-assigned this May 4, 2023

islamaliev added feature New feature or request area/api Related to the external API component labels May 4, 2023

islamaliev added this to the DefraDB v0.6 milestone May 4, 2023

hound bot reviewed May 4, 2023

View reviewed changes

islamaliev requested review from jsimnz, fredcarle and AndrewSisley May 4, 2023 16:18

AndrewSisley reviewed May 4, 2023

View reviewed changes

client/collection.go Outdated Show resolved Hide resolved

AndrewSisley reviewed May 4, 2023

View reviewed changes

core/key.go Outdated Show resolved Hide resolved

AndrewSisley reviewed May 4, 2023

View reviewed changes

core/key.go Outdated Show resolved Hide resolved

islamaliev force-pushed the islam/feat/297-non-unique-secondary-index branch from 6e13d5a to 5fae460 Compare May 19, 2023 07:11

islamaliev force-pushed the islam/feat/297-non-unique-secondary-index branch 2 times, most recently from 6710210 to 106f7e0 Compare May 30, 2023 15:32

islamaliev marked this pull request as ready for review May 30, 2023 15:35

islamaliev requested a review from AndrewSisley May 30, 2023 15:35

jsimnz changed the title ~~Islam/feat/297 non unique secondary index~~ feat: Non-unique secondary index May 31, 2023

islamaliev force-pushed the islam/feat/297-non-unique-secondary-index branch from 132dc53 to 53391ae Compare May 31, 2023 10:44

AndrewSisley requested changes May 31, 2023

View reviewed changes

fredcarle requested changes May 31, 2023

View reviewed changes

AndrewSisley reviewed May 31, 2023

View reviewed changes

db/collection.go Outdated Show resolved Hide resolved

AndrewSisley requested changes May 31, 2023

View reviewed changes

db/index.go Outdated Show resolved Hide resolved

db/index.go Outdated Show resolved Hide resolved

db/index.go Outdated Show resolved Hide resolved

request/graphql/schema/collection.go Outdated Show resolved Hide resolved

request/graphql/schema/collection.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Jun 7, 2023

View reviewed changes

db/collection_index.go Outdated Show resolved Hide resolved

islamaliev force-pushed the islam/feat/297-non-unique-secondary-index branch from 19dff9e to 35802a4 Compare June 12, 2023 19:49

AndrewSisley reviewed Jun 12, 2023

View reviewed changes

tests/integration/schema/updates/index/simple_test.go Show resolved Hide resolved

AndrewSisley requested changes Jun 12, 2023

View reviewed changes

shahzadlone previously requested changes Jun 13, 2023

View reviewed changes

shahzadlone reviewed Jun 13, 2023

View reviewed changes

core/key.go Show resolved Hide resolved

islamaliev added 13 commits June 19, 2023 16:00

Polish

c0343ad

Add missing test

448cf93

Store indexed values using CBOR encoding

6e01caf

Rely on CBOR's nil-value distinction instead of manual

Remove index cache from collection

806602d

Correctly implement Txn

Add integration test for dropping non-existing index

93cae45

Move directive consts

3b445c3

Add @index directive to graphql schema

3af3fd2

Move newFetcher

4e639c2

Split tests

4560894

Add integration tests for getting indexes

9793ac2

Update copyright

a635cb1

Add a comment

feb420b

Format

6e24502

islamaliev force-pushed the islam/feat/297-non-unique-secondary-index branch from f35622d to 6e24502 Compare June 19, 2023 14:00

islamaliev requested review from fredcarle, shahzadlone and AndrewSisley June 19, 2023 14:01

AndrewSisley approved these changes Jun 19, 2023

View reviewed changes

fredcarle requested changes Jun 19, 2023

View reviewed changes

db/collection_index.go Show resolved Hide resolved

db/collection_index.go Show resolved Hide resolved

db/collection_index.go Show resolved Hide resolved

islamaliev added 2 commits June 19, 2023 17:11

Add documentation to public methods

bfcd6b7

Make checking fields case sensitive

5b4c60d

islamaliev requested a review from fredcarle June 19, 2023 15:17

islamaliev added 2 commits June 19, 2023 17:27

Add more documentation

b458b6d

Add missing test cases

3919472

fredcarle approved these changes Jun 19, 2023

View reviewed changes

islamaliev merged commit be619fd into develop Jun 19, 2023

islamaliev deleted the islam/feat/297-non-unique-secondary-index branch June 19, 2023 18:44

islamaliev mentioned this pull request Oct 17, 2023

feat: Remove CollectionDescription.Schema #1965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Non-unique secondary index #1450

feat: Non-unique secondary index #1450

islamaliev commented May 4, 2023 •

edited

Loading

codecov bot commented May 4, 2023 •

edited

Loading

AndrewSisley left a comment

AndrewSisley left a comment

fredcarle left a comment

AndrewSisley left a comment

AndrewSisley left a comment

AndrewSisley left a comment

fredcarle left a comment

fredcarle left a comment

feat: Non-unique secondary index #1450

feat: Non-unique secondary index #1450

Conversation

islamaliev commented May 4, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented May 4, 2023 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

islamaliev commented May 4, 2023 •

edited

Loading

codecov bot commented May 4, 2023 •

edited

Loading