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.

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

Support for composite indexes #376

Merged
merged 40 commits into from
May 1, 2021
Merged

Support for composite indexes #376

merged 40 commits into from
May 1, 2021

Conversation

jhchabran
Copy link
Collaborator

@jhchabran jhchabran commented Mar 18, 2021

This pull-request adds support for composite indexes. It uses document.Array to wrap the multiple values into one for the index stores. The changes are not that large as the line changes count suggests, this is mainly due to having added a lot of table tests on the indexes.

I have also rewritten the tests for the index iteration methods, which are now quite exhaustive.

The Benchmarks are here and are quite good (0% change on simple indexes).

database/catalog.go Outdated Show resolved Hide resolved
database/catalog.go Outdated Show resolved Hide resolved
database/catalog_test.go Outdated Show resolved Hide resolved
database/config.go Outdated Show resolved Hide resolved
database/config.go Outdated Show resolved Hide resolved
planner/optimizer.go Outdated Show resolved Hide resolved
planner/optimizer.go Outdated Show resolved Hide resolved
query/select_test.go Outdated Show resolved Hide resolved
database/index.go Outdated Show resolved Hide resolved
@jhchabran jhchabran force-pushed the compindex branch 3 times, most recently from 22206f7 to d65c079 Compare March 19, 2021 13:03
@codecov-io
Copy link

codecov-io commented Mar 19, 2021

Codecov Report

Merging #376 (de1363d) into main (e19ca7a) will increase coverage by 0.26%.
The diff coverage is 77.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
+ Coverage   68.53%   68.80%   +0.26%     
==========================================
  Files          74       74              
  Lines        7479     7716     +237     
==========================================
+ Hits         5126     5309     +183     
- Misses       1678     1717      +39     
- Partials      675      690      +15     
Impacted Files Coverage Δ
document/document.go 65.18% <0.00%> (-0.74%) ⬇️
expr/comparison.go 70.32% <0.00%> (-3.24%) ⬇️
database/table.go 51.72% <61.11%> (+0.46%) ⬆️
database/index.go 71.21% <77.00%> (+1.12%) ⬆️
planner/optimizer.go 78.36% <79.84%> (+1.39%) ⬆️
stream/iterator.go 66.13% <80.26%> (+2.80%) ⬆️
database/config.go 66.95% <83.33%> (+1.38%) ⬆️
database/catalog.go 81.38% <86.66%> (+0.29%) ⬆️
query/create.go 84.61% <100.00%> (ø)
sql/parser/create.go 75.88% <100.00%> (-0.29%) ⬇️
... and 1 more

Continue to review full report at Codecov.

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

@jhchabran jhchabran force-pushed the compindex branch 3 times, most recently from 5ff11fe to ebfd78c Compare April 5, 2021 17:23
@jhchabran jhchabran marked this pull request as ready for review April 5, 2021 17:43
planner/optimizer.go Outdated Show resolved Hide resolved
database/index.go Outdated Show resolved Hide resolved
database/index.go Outdated Show resolved Hide resolved
database/index.go Outdated Show resolved Hide resolved
database/index.go Outdated Show resolved Hide resolved
database/index.go Show resolved Hide resolved
database/index.go Outdated Show resolved Hide resolved
@asdine asdine added this to the v0.12.0 milestone Apr 12, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2021

Codecov Report

Merging #376 (d404b59) into main (e19ca7a) will decrease coverage by 0.58%.
The diff coverage is 58.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
- Coverage   68.53%   67.95%   -0.59%     
==========================================
  Files          74       75       +1     
  Lines        7479     7805     +326     
==========================================
+ Hits         5126     5304     +178     
- Misses       1678     1800     +122     
- Partials      675      701      +26     
Impacted Files Coverage Δ
document/array.go 60.86% <0.00%> (-6.72%) ⬇️
document/document.go 65.18% <0.00%> (-0.74%) ⬇️
document/value.go 73.23% <0.00%> (ø)
expr/comparison.go 70.32% <0.00%> (-3.24%) ⬇️
stream/range.go 34.04% <34.04%> (ø)
database/table.go 51.42% <48.27%> (+0.17%) ⬆️
database/index.go 65.21% <65.11%> (-4.87%) ⬇️
planner/optimizer.go 76.94% <80.89%> (-0.04%) ⬇️
database/config.go 66.95% <83.33%> (+1.38%) ⬆️
database/catalog.go 81.38% <86.66%> (+0.29%) ⬆️
... and 9 more

Continue to review full report at Codecov.

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

@jhchabran jhchabran marked this pull request as ready for review April 26, 2021 17:32
Copy link
Collaborator

@asdine asdine left a comment

Choose a reason for hiding this comment

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

I'm really excited about this, thanks @jhchabran !
I'm approving to unblock, feel free to merge once you feel it's ready

database/index.go Outdated Show resolved Hide resolved
database/index.go Outdated Show resolved Hide resolved
planner/optimizer.go Show resolved Hide resolved
database/catalog.go Outdated Show resolved Hide resolved
cmd/genji/dbutil/dump_test.go Outdated Show resolved Hide resolved
database/config.go Outdated Show resolved Hide resolved
database/table.go Outdated Show resolved Hide resolved
query/reindex_test.go Outdated Show resolved Hide resolved
sql/parser/create.go Show resolved Hide resolved
// what the index says this node type must be
typ := idx.Info.Types[i]

fno.v, ok, err = operandCanUseIndex(typ, fno.path, info.FieldConstraints, fno.v)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed as it's already done above right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We cannot, because the only other one is just for the primary key because it builds the candidate on the fly.

jhchabran and others added 2 commits April 29, 2021 17:09
Co-authored-by: Asdine El Hrychy <asdine.elhrychy@gmail.com>
@asdine asdine merged commit fffcc2a into chaisql:main May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants