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

refactor: Generate field ids using a sequence #2339

Merged
merged 8 commits into from
Feb 28, 2024

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #2333

Description

Generates field ids using a sequence instead of their index in the array.

This is required for allowing field deletion and schema branching as otherwise those two will alter the position and thus id of fields. For example, previously if two schema branches were to add a new field each, the new fields would share the same id and mess the datastore cache up in interesting ways.

WARNING

This PR is currently branched off of #2336 - please only review commits from 'Simplify isNew evaluation' onwards here.

@AndrewSisley AndrewSisley added area/collections Related to the collections system refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Feb 23, 2024
@AndrewSisley AndrewSisley added this to the DefraDB v0.10 milestone Feb 23, 2024
@AndrewSisley AndrewSisley requested a review from a team February 23, 2024 22:31
@AndrewSisley AndrewSisley self-assigned this Feb 23, 2024
Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

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

Overall looks great. I just have few comments.

client/definitions.go Outdated Show resolved Hide resolved
client/definitions.go Outdated Show resolved Hide resolved
client/index.go Outdated Show resolved Hide resolved
planner/mapper/mapper.go Outdated Show resolved Hide resolved

// FieldIDSequenceKey is used to key the sequence used to generate field ids.
//
// The sequence is specific to each collection root. Multiple collection of the same root
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Can you remind me what the root is? Do you mean the root schema?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind. I saw the answer bellow 😅

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.

LGTM!

@AndrewSisley AndrewSisley changed the title refactor: Generates field ids using a sequence refactor: Generate field ids using a sequence Feb 28, 2024
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

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

Project coverage is 74.83%. Comparing base (db75564) to head (7ba6239).

❗ Current head 7ba6239 differs from pull request most recent head a6dfa76. Consider uploading reports for the commit a6dfa76 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2339      +/-   ##
===========================================
+ Coverage    74.82%   74.83%   +0.01%     
===========================================
  Files          257      257              
  Lines        25314    25349      +35     
===========================================
+ Hits         18940    18968      +28     
- Misses        5083     5090       +7     
  Partials      1291     1291              
Flag Coverage Δ
all-tests 74.83% <71.43%> (+0.01%) ⬆️

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

Files Coverage Δ
db/collection_index.go 91.02% <100.00%> (+0.11%) ⬆️
db/db.go 64.23% <100.00%> (ø)
db/lens.go 69.72% <100.00%> (ø)
db/sequence.go 68.42% <100.00%> (+4.14%) ⬆️
core/key.go 86.61% <75.00%> (-0.66%) ⬇️
db/collection.go 71.13% <64.58%> (-1.08%) ⬇️

... and 7 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 db75564...a6dfa76. Read the comment docs.

@AndrewSisley AndrewSisley merged commit 9248cbc into sourcenetwork:develop Feb 28, 2024
27 of 28 checks passed
@AndrewSisley AndrewSisley deleted the 2333-field-id-seq branch February 28, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/collections Related to the collections system refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FieldDescription IDs are dependent on which fields are currently on the schema
3 participants