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

feat: Add dockey field for commit field #1216

Merged
merged 21 commits into from
Mar 24, 2023
Merged

feat: Add dockey field for commit field #1216

merged 21 commits into from
Mar 24, 2023

Conversation

islamaliev
Copy link
Contributor

@islamaliev islamaliev commented Mar 22, 2023 •
edited
Loading

Relevant issue(s)

Resolves #846

Description

This PR add 'dockey' property to commit field. It also enables grouping and ordering commits by 'dockey'
This PR introduces a BREAKING CHANGES because it changes the way deltas are stored and as such will invalidate all previously stored CIDs for those deltas.

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?

integration tests

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

  • MacOS

@islamaliev islamaliev added area/query Related to the query component area/api Related to the external API component labels Mar 22, 2023
@islamaliev islamaliev added this to the DefraDB v0.5 milestone Mar 22, 2023
@islamaliev islamaliev self-assigned this Mar 22, 2023
@islamaliev islamaliev force-pushed the islam/feat/dockey-field-for-commit-field branch from 5e2696e to 2e32405 Compare March 22, 2023 11:17
@codecov
Copy link

codecov bot commented Mar 22, 2023 •
edited
Loading

Codecov Report

Merging #1216 (9d4f70d) into develop (4a3c120) will increase coverage by 0.02%.
The diff coverage is 82.74%.

❗ Current head 9d4f70d differs from pull request most recent head dd6c9a4. Consider uploading reports for the commit dd6c9a4 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1216      +/-   ##
===========================================
+ Coverage    68.57%   68.60%   +0.02%     
===========================================
  Files          181      181              
  Lines        17125    17144      +19     
===========================================
+ Hits         11744    11761      +17     
- Misses        4423     4424       +1     
- Partials       958      959       +1
Impacted Files Coverage Δ
client/dockey.go 51.16% <ø> (ø)
core/key.go 86.61% <ø> (ø)
planner/errors.go 0.00% <ø> (ø)
planner/multi.go 76.25% <40.00%> (ø)
merkle/clock/ipld.go 26.86% <50.00%> (ø)
request/graphql/schema/collection.go 81.72% <50.00%> (ø)
merkle/clock/clock.go 65.74% <66.66%> (ø)
db/collection.go 68.49% <71.42%> (-0.24%) ⬇️
planner/commit.go 81.11% <75.00%> (-2.42%) ⬇️
planner/planner.go 77.38% <89.65%> (+0.95%) ⬆️
... and 7 more

... and 5 files with indirect coverage changes

@source-devs
Copy link

Benchmark Results

Summary

  • 0 Benchmarks successfully compared.
  • 0 Benchmarks were ✅ Better.
  • 0 Benchmarks were ❌ Worse .
  • 0 Benchmarks were ✨ Unchanged.
✅ See Better Results...
time/opdelta
 
❌ See Worse Results...
time/opdelta
 
✨ See Unchanged Results...
time/opdelta
 
🐋 See Full Results...

1 similar comment
@source-devs
Copy link

Benchmark Results

Summary

  • 0 Benchmarks successfully compared.
  • 0 Benchmarks were ✅ Better.
  • 0 Benchmarks were ❌ Worse .
  • 0 Benchmarks were ✨ Unchanged.
✅ See Better Results...
time/opdelta
 
❌ See Worse Results...
time/opdelta
 
✨ See Unchanged Results...
time/opdelta
 
🐋 See Full Results...

@jsimnz jsimnz added the action/no-benchmark Skips the action that runs the benchmark. label Mar 22, 2023
fredcarle
fredcarle approved these changes Mar 22, 2023
View reviewed changes
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'm ok with the changes in this PR but please wait for at least an other person to review as well.

There are many changes in the PR that are not related to the main issue being worked on. I would suggest opening a separate PR next time for those code-quality changes as having so much of it mixed in can unnecessarily delay merging the main changes because people are arguing the secondary changes.

merkle/clock/clock.go Outdated Show resolved Hide resolved
@fredcarle fredcarle requested a review from a team March 22, 2023 16:07
AndrewSisley
AndrewSisley requested changes Mar 22, 2023
View reviewed changes
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.

I've not looked at the prod code yet, but I thought I'd leave feedback on the tests for you to have a look at in the meantime. Coverage looks pretty good, thanks :) Is one major discussion to be resolved, and a few smaller tasks so far.

Thank you very much for converting so many tests to the new framework - it is a very boring task.

tests/integration/query/commits/with_group_test.go Outdated Show resolved Hide resolved
tests/integration/query/commits/with_group_test.go Outdated Show resolved Hide resolved
tests/integration/query/latest_commits/with_dockey_test.go Outdated Show resolved Hide resolved
tests/integration/query/simple/with_version_test.go Show resolved Hide resolved
tests/integration/query/simple/with_version_test.go Outdated Show resolved Hide resolved
tests/integration/query/commits/with_depth_test.go Outdated Show resolved Hide resolved
AndrewSisley
AndrewSisley requested changes Mar 22, 2023
View reviewed changes
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.

Production code mostly looks good, but I just want to reiterate Fred's point - by including so much other stuff, especially stuff like renames which involve lots of safe, boring line changes that can be glossed over, it drowns out the actual changes that you have made.

This can be compensated for to some extent by a clean commit history, but that only goes so far and doesn't solve the problem that some of the other changes can slow down the merge of the core feature in this PR (via discussion/bugs/etc).

You also appear to have a (production) bug (in planner/commit.go), and I think the tests may only be passing accidentally - possibly due to the presence of another, previously hidden bug.

client/dockey.go Show resolved Hide resolved
planner/commit.go Show resolved Hide resolved
planner/commit.go Outdated Show resolved Hide resolved
shahzadlone
shahzadlone reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@shahzadlone shahzadlone left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I found it a bit distracting as a reviewer to comb through things that weren't relevant to this PR (i.e. the renaming of variables that were not related to the change).

Would suggest in future to do these unrelated factoring in a separate PR. For factoring that is somewhat related but not the actual feature I would like for a way to know as reviewer (commit message, PR description or something else).

Also slight disagreement in changing names to their type names (IMO yes we can name them better but naming them after their type isn't always the better name).

@jsimnz
Copy link
Member

jsimnz commented Mar 22, 2023

+1 on the confusion for the various unrelated renames.

AndrewSisley
AndrewSisley approved these changes Mar 24, 2023
View reviewed changes
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.

Looks good to me! Thank you for tolerating the lack of test helper functions for now. Might be worth quickly checking in with Shahzad in case he is particularly bothered by any of the name changes (and not just their inclusion in this PR) if you haven't already before merge

docs/data_format_changes/i846-add-dockey-to-blocks.md Outdated Show resolved Hide resolved
@islamaliev islamaliev force-pushed the islam/feat/dockey-field-for-commit-field branch from 9d4f70d to dd6c9a4 Compare March 24, 2023 16:06
@islamaliev islamaliev merged commit 0cddca5 into develop Mar 24, 2023
@islamaliev islamaliev deleted the islam/feat/dockey-field-for-commit-field branch March 24, 2023 16:31
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
@islamaliev @shahzadlone
feat: Add dockey field for commit field (#1216)
1208b5d 
Add dockey field for commit field

Dockey was not stored for composite deltas, with this change we store dockey in the following format /collection_id/instance_type/key/field_id.
For field deltas dockey was stored but without instance_type. This change add missing intance_type to field delta.

Adjust CIDs in all tests
Add breaking change file
Switch some tests to use new framework
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@islamaliev
feat: Add dockey field for commit field (sourcenetwork#1216)
e097d3e 
Add dockey field for commit field

Dockey was not stored for composite deltas, with this change we store dockey in the following format /collection_id/instance_type/key/field_id.
For field deltas dockey was stored but without instance_type. This change add missing intance_type to field delta.

Adjust CIDs in all tests
Add breaking change file
Switch some tests to use new framework
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shahzadlone shahzadlone shahzadlone left review comments

@fredcarle fredcarle fredcarle approved these changes

@AndrewSisley AndrewSisley AndrewSisley approved these changes

Assignees

@islamaliev islamaliev

Labels
action/no-benchmark Skips the action that runs the benchmark. area/api Related to the external API component area/query Related to the query component
Projects
None yet
Milestone
DefraDB v0.5
Development

Successfully merging this pull request may close these issues.

Commits: Add dockey field to commit fields
6 participants
@islamaliev @source-devs @jsimnz @fredcarle @shahzadlone @AndrewSisley