Replace core.Key and ds.Key with strongly typed data structures #19

AndrewSisley · 2021-10-29T16:08:16Z

Replace the string based core.Key and ds.Key types with strongly typed data structures.

Namespacing is now entirely handled by the datastores and should not be present on the keys (i.e. no leading '/db/data').

Fixes the factory_test.go tests, only failing tests are failing on master too (Test_Generator_buildTypesFromAST_SingleScalarField, TestMutationParse_Update_Simple_Array and TestMutationParse_Create_Error_Missing_Data).

Database dumps seem to pair up with master (manually checked a few of the db/tests, and my local file-based instance).

Performance doesn't seem to have taken a hit (checked manually firing in queries via UI client, might even be improved due to removal of a few string manipulations - query time was 250-680 micro seconds), but maybe have an eye out for anything silly I may have done in the planner in particular.

System will likely need further extension in the time-traveling branch, and when implementing the non-primary index stuff.

WithPriorityFlag is called within reg.getPriority and should not be the responsibility of the caller

Is not needed and complicates the code

WithValueFlag is called in writeObjectMarker

todo · 2021-10-29T16:08:19Z

unit test this like crazy

defradb/core/key.go

Lines 85 to 90 in ecc6049

    
           func NewDataStoreKey(key string) DataStoreKey { //@todo: unit test this like crazy 
        
           	dataStoreKey := DataStoreKey{} 
        
           	if key == "" { 
        
           		return dataStoreKey 
        
           	}

This comment was generated by todo based on a `todo` comment in `ecc6049` in #19. cc @sourcenetwork.

todo · 2021-10-29T16:08:22Z

Support multiple spans

defradb/db/fetcher/fetcher.go

Lines 115 to 118 in ecc6049

    
           	Prefix: spans[0].Start().ToString(), // @todo: Support multiple spans 
        
           } 
        
           if df.reverse { 
        
           	q.Orders = []dsq.Order{dsq.OrderByKeyDescending{}}

This comment was generated by todo based on a `todo` comment in `ecc6049` in #19. cc @sourcenetwork.

todo · 2021-10-29T16:08:25Z

, this is very lazy

defradb/db/fetcher/fetcher.go

Lines 226 to 229 in ecc6049

    
           df.doc.Key = []byte(kv.Key.DocKey) // core.DataStoreKey{DocKey: kv.Key.DocKey}.Bytes() //todo, this is very lazy 
        
           // keyFD := df.schemaFields[0] // _key 
        
           // df.doc.Properties[keyFD] = &document.EncProperty{ 
        
           // 	Raw: df.doc.Key[:],

This comment was generated by todo based on a `todo` comment in `ecc6049` in #19. cc @sourcenetwork.

todo · 2021-10-29T16:08:28Z

Config logger param package wide

defradb/merkle/clock/clock.go

Lines 42 to 45 in ecc6049

    
           		headset:   newHeadset(headstore, namespace), //TODO: Config logger param package wide 
        
           		crdt:      crdt, 
        
           	} 
        
           }

This comment was generated by todo based on a `TODO` comment in `ecc6049` in #19. cc @sourcenetwork.

AndrewSisley · 2021-10-29T20:10:58Z

I think I have actually done something horrible, PR description benchmark were invalid as I forgot you need to re-run go install, newer benchmarks are considerably slower and I need to investigate before merge (current guess is I botched the span/PrefixWith stuff in planner).

jsimnz · 2021-11-01T05:29:47Z

ill check the perf on my side and investigate. TBD

AndrewSisley · 2021-11-01T18:35:49Z

I think I just got lucky running the queries from master the first few times. Performance seems comparable ~500us - 2ms to master. Added a couple of commits (including one fixup) whilst going through again today. I will rebase onto master and squash the fixup before merging after/if the PR is approved

Is wasteful taking the whole struct (and initing one in most cases)

Is simpler

jsimnz · 2021-11-05T18:56:02Z

I think at the moment, I'd rather just implement the simpler version of the conversion, instead of the structured version.

It kinda seems like the same problems plauge the implementation here, specifically in the New constructors, it assumes certain structures, and does a lot to attempt to make sure the format is consistent, while technically not giving us more assurance then the generic version. Unless of course we manually construct the key structs each field at a time.

Thoughts?

jsimnz · 2021-11-05T18:56:45Z

core/key.go

+// [CollectionId]/[PrimaryIndexId]/
+//
+// Any properties before the above (assuming a '/' deliminator) are ignored
+func NewDataStoreKey(key string) DataStoreKey {


Specifically these constructors is what im referring to

Would be very much in favour of merging lol :) But I can see where you are coming from - I spent quite a while trying to get rid of as many prod-code references to those constructors as possible.

Those constructors are only called in a small number of places (ignoring tests and the deprecated get.go, 3 for NewDataStoreKey, and once for the HeadStoreKey. And those are all on direct output from the datastore where the keys should be 100% complete. Everything else is manually built.

I think even if these were more widely used (which they shouldn't be), the review still solves the following:

Code is no longer order dependent - e.g. if something in place x changes the structure by (e.g.) adding the dockey, then place y wont break in really weird ways because of the string parsing hacks it does to get the collection id based on the number of '/' present

It is now much more obvious what is being used and where, instead of having to track back through the object's lifetime and navigate through the ds.Namespace()/Base/Split/whatever you can see that x is the CollectionId with IndexId

The format of string output is much more visible and you can now see what is being written/read

Removes the db/namespace from the vars being passed through the codebase, forcing the datastores to be responsible for managing that and reducing the 'stuff' the devs see when debugging. I also found that their inclusion within the string-key was partial and very dependent on where you were in the codebase, not all places had them and some broke when were given them.

It also keeps all the scary stuff in the one place, where it is very visible, and tested. There is a risk that we'll accidentally call these functions when we shouldn't (i.e. when there is no reason to, and we can manually set stuff), but it is better than having poorly documented structured-strings floating round all over the place :P

jsimnz · 2021-11-08T14:24:01Z

These are all pretty valid reasons :). And the benefits are fairly clear. I haven't checked yet, but does this play well with the actual data stores? When you call txn.headstore.get(/my-doc-key) the 'headstore' references a "Wrapped data store" from the go-datastore package. Which automatically handles prefix wrapping and stripping for the "/db/heads" prefix. Same applies to /db/data, /db/blocks, and /db/system.

…

On Mon, Nov 8, 2021, 9:17 AM AndrewSisley ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/key.go <#19 (comment)>: > +} + +// Creates a new DataStoreKey from a string as best as it can, +// splitting the input using '/' and ':' as field deliminaters. It assumes +// that the input string is in one of the following formats: +// +// [DocKey] +// [DocKey]:[InstanceType] +// [DocKey]/[FieldId] +// [DocKey]/[FieldId]:[InstanceType] +// Any of the above prefixed but with one of the below: +// [PrimaryIndexId]/ +// [CollectionId]/[PrimaryIndexId]/ +// +// Any properties before the above (assuming a '/' deliminator) are ignored +func NewDataStoreKey(key string) DataStoreKey { Would be very much in favour of merging lol :) But I can see where you are coming from - I spent quite a while trying to get rid of as many prod-code references to those constructors as possible. Those constructors are only called in a small number of places (ignoring tests and the deprecated get.go, 3 for NewDataStoreKey, and once for the HeadStoreKey. And those are all on direct output from the datastore where the keys should be 100% complete. Everything else is manually built. I think even if these were more widely used (which they shouldn't be), the review still solves the following: 1. Code is no longer order dependent - e.g. if something in place x changes the structure by (e.g.) adding the dockey, then place y wont break in really weird ways because of the string parsing hacks it does to get the collection id based on the number of '/' present 2. It is now much more obvious what is being used and where, instead of having to track back through the object's lifetime and navigate through the ds.Namespace()/Base/Split/whatever you can see that x is the CollectionId with IndexId 3. The format of string output is much more visible and you can now see what is being written/read 4. Removes the db/namespace from the vars being passed through the codebase, forcing the datastores to be responsible for managing that and reducing the 'stuff' the devs see when debugging. I also found that their inclusion within the string-key was partial and very dependent on where you were in the codebase, not all places had them and some broke when were given them. It also keeps all the scary stuff in the one place, where it is very visible, and tested. There is a risk that we'll accidentally call these functions when we shouldn't (i.e. when there is no reason to, and we can manually set stuff), but it is better than having poorly documented structured-strings floating round all over the place :P — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#19 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6UKNUVRBVTGSXW5AQHCILUK7LWVANCNFSM5G7VMXSQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

AndrewSisley · 2021-11-08T14:48:32Z

Code relies on the datastore wrapping - I paid quite a lot of attention to the db dumps during testing to make sure I wasn't losing that.

These are all pretty valid reasons :). And the benefits are fairly clear. I haven't checked yet, but does this play well with the actual data stores? When you call txn.headstore.get(/my-doc-key) the 'headstore' references a "Wrapped data store" from the go-datastore package. Which automatically handles prefix wrapping and stripping for the "/db/heads" prefix. Same applies to /db/data, /db/blocks, and /db/system.

jsimnz · 2021-11-08T21:21:16Z

OK, I'm inclined to go with this approach, however with this much string manipulation/allocation in the hot path, I want to get at least a rudimentary benchmark suite in place to ensure everything is good without notable (or ideally any) perf degredation.

AndrewSisley · 2021-11-08T21:26:46Z

Okay fair enough :) I'll probs get cracking on that (after I finish the group stuff) if you don't beat me to it :D

AndrewSisley · 2021-12-08T15:14:42Z

Closing this PR as Github doesnt seem to allow me to change the target branch once a PR is open. Will open a new one against develop.

AndrewSisley added 11 commits October 25, 2021 14:46

Reference correct Key type in documentation

ebcde6b

Remove commented out import

b1d5025

Remove commented out import

8bb1b5a

Remove commented out code

fbf4a67

Remove commented out code

6550e6b

Remove commented out import

8648e9a

Replace references to ds.Key with core.Key

0228369

Replace ds.Key with structured data type

a5d7489

Remove duplicate call

9bd08fa

WithPriorityFlag is called within reg.getPriority and should not be the responsibility of the caller

Remove DataStoreKey from Collection struct

2265a65

Is not needed and complicates the code

Remove duplicate call

ecc6049

WithValueFlag is called in writeObjectMarker

AndrewSisley requested a review from jsimnz October 29, 2021 16:08

AndrewSisley self-assigned this Oct 29, 2021

AndrewSisley mentioned this pull request Oct 29, 2021

Issue 17: Convert ds key to core key #18

Closed

fixup! Replace ds.Key with structured data type

c1b4ff1

AndrewSisley added 2 commits November 1, 2021 14:46

Take docKey string as param instead of DataStoreKey

274bb49

Is wasteful taking the whole struct (and initing one in most cases)

Replace documentFetcher indexKey with bool

8cbf009

Is simpler

AndrewSisley force-pushed the merkle-crdt-factory-key-fix-without-store-comp branch from ef8c7aa to 8cbf009 Compare November 1, 2021 18:47

FIXUP - add key tests

46b4871

jsimnz reviewed Nov 5, 2021

View reviewed changes

jsimnz linked an issue Nov 17, 2021 that may be closed by this pull request

Convert all instances of ds.Key to core.Key in both internal and external APIs #17

Closed

1 task

AndrewSisley closed this Dec 8, 2021

AndrewSisley mentioned this pull request Dec 8, 2021

refactor: Strongly typed key refactor (#17) #84

Merged

AndrewSisley deleted the merkle-crdt-factory-key-fix-without-store-comp branch February 7, 2022 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace core.Key and ds.Key with strongly typed data structures #19

Replace core.Key and ds.Key with strongly typed data structures #19

AndrewSisley commented Oct 29, 2021

todo bot commented Oct 29, 2021

todo bot commented Oct 29, 2021

todo bot commented Oct 29, 2021

todo bot commented Oct 29, 2021

AndrewSisley commented Oct 29, 2021

jsimnz commented Nov 1, 2021

AndrewSisley commented Nov 1, 2021

jsimnz commented Nov 5, 2021

jsimnz Nov 5, 2021

AndrewSisley Nov 8, 2021

jsimnz commented Nov 8, 2021 via email

AndrewSisley commented Nov 8, 2021

jsimnz commented Nov 8, 2021

AndrewSisley commented Nov 8, 2021

AndrewSisley commented Dec 8, 2021

Replace core.Key and ds.Key with strongly typed data structures #19

Replace core.Key and ds.Key with strongly typed data structures #19

Conversation

AndrewSisley commented Oct 29, 2021

todo bot commented Oct 29, 2021

unit test this like crazy

This comment was generated by todo based on a todo comment in ecc6049 in #19. cc @sourcenetwork.

todo bot commented Oct 29, 2021

Support multiple spans

This comment was generated by todo based on a todo comment in ecc6049 in #19. cc @sourcenetwork.

todo bot commented Oct 29, 2021

, this is very lazy

This comment was generated by todo based on a todo comment in ecc6049 in #19. cc @sourcenetwork.

todo bot commented Oct 29, 2021

Config logger param package wide

This comment was generated by todo based on a TODO comment in ecc6049 in #19. cc @sourcenetwork.

AndrewSisley commented Oct 29, 2021

jsimnz commented Nov 1, 2021

AndrewSisley commented Nov 1, 2021

jsimnz commented Nov 5, 2021

jsimnz Nov 5, 2021

Choose a reason for hiding this comment

AndrewSisley Nov 8, 2021

Choose a reason for hiding this comment

jsimnz commented Nov 8, 2021 via email

AndrewSisley commented Nov 8, 2021

jsimnz commented Nov 8, 2021

AndrewSisley commented Nov 8, 2021

AndrewSisley commented Dec 8, 2021

This comment was generated by todo based on a `todo` comment in `ecc6049` in #19. cc @sourcenetwork.

This comment was generated by todo based on a `todo` comment in `ecc6049` in #19. cc @sourcenetwork.

This comment was generated by todo based on a `todo` comment in `ecc6049` in #19. cc @sourcenetwork.

This comment was generated by todo based on a `TODO` comment in `ecc6049` in #19. cc @sourcenetwork.