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

feat: Reverted order for indexed fields #2335

Merged

Conversation

islamaliev
Copy link
Contributor

Relevant issue(s)

Resolves #2229

Description

Enable reverted ordering for indexed fields. Which is mostly relevant to composite indexes.

This PR introduces a whole new package encoding. A significant part of it is taken from CocroachDB which fits well to our needs. Other encoding approaches (mostly for integers) were also consider: like fixed-length encoding, avro's zizzag encoding, base128 varints encoding and few others.

@islamaliev islamaliev requested a review from a team February 21, 2024 16:46
@islamaliev islamaliev self-assigned this Feb 21, 2024
@islamaliev islamaliev added feature New feature or request area/query Related to the query component area/datastore Related to the datastore / storage engine system perf Performance issue or suggestion labels Feb 21, 2024
@islamaliev islamaliev added this to the DefraDB v0.10 milestone Feb 21, 2024
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 88.63388% with 104 lines in your changes are missing coverage. Please review.

Project coverage is 75.05%. Comparing base (9248cbc) to head (ad348a8).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2335      +/-   ##
===========================================
+ Coverage    74.75%   75.05%   +0.30%     
===========================================
  Files          257      266       +9     
  Lines        25349    25855     +506     
===========================================
+ Hits         18949    19404     +455     
- Misses        5103     5147      +44     
- Partials      1297     1304       +7     
Flag Coverage Δ
all-tests 75.05% <88.63%> (+0.30%) ⬆️

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

Files Coverage Δ
client/document.go 70.64% <100.00%> (ø)
client/field.go 100.00% <100.00%> (+33.33%) ⬆️
client/index.go 80.95% <ø> (ø)
core/key.go 84.53% <100.00%> (-2.07%) ⬇️
db/collection_delete.go 32.77% <100.00%> (ø)
db/collection_index.go 90.85% <100.00%> (-0.16%) ⬇️
db/errors.go 61.09% <ø> (ø)
db/fetcher/encoded_doc.go 76.14% <100.00%> (ø)
db/index.go 67.92% <100.00%> (+0.96%) ⬆️
encoding/bytes.go 100.00% <100.00%> (ø)
... and 18 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 9248cbc...ad348a8. Read the comment docs.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Is going to take a while for me to get through this one :) Submitting first chunk of comments now.

core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
client/value.go Outdated Show resolved Hide resolved
client/value.go Outdated Show resolved Hide resolved
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Pausing my review for now - there appears to be a lot of dead code, and a lot of fairly fiddly and undocumented code. It looks like you have done a great job getting it to work, but I do not think it is yet ready for review.

Can you please remove the dead code, and document the remaining. Please in the code comments focus on why what is being done is being done.

db/encoding/bytes.go Outdated Show resolved Hide resolved
db/encoding/bytes.go Outdated Show resolved Hide resolved
db/encoding/bytes.go Outdated Show resolved Hide resolved
db/encoding/string.go Outdated Show resolved Hide resolved
db/encoding/bytes.go Outdated Show resolved Hide resolved
db/encoding/bytes.go Outdated Show resolved Hide resolved
core/key_test.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

I like the changes. I'm wondering if the encoding should eventually be applied to stored values instead of using cbor.

Approving with a few minor comments to resolve before merge.

client/index_test.go Outdated Show resolved Hide resolved
core/encoding.go Outdated Show resolved Hide resolved
core/key.go Outdated Show resolved Hide resolved
db/collection_delete.go Outdated Show resolved Hide resolved
db/collection_index.go Outdated Show resolved Hide resolved
db/index_test.go Show resolved Hide resolved
encoding/bytes.go Show resolved Hide resolved
@islamaliev islamaliev dismissed AndrewSisley’s stale review March 1, 2024 21:18

it's discussed with him

@islamaliev islamaliev merged commit b730d3f into sourcenetwork:develop Mar 1, 2024
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datastore Related to the datastore / storage engine system area/query Related to the query component feature New feature or request perf Performance issue or suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sec. indexes: enable order direction for composite indexes
4 participants