fix: Add object marker to enable return of empty docs #800

fredcarle · 2022-09-15T08:14:53Z

Relevant issue(s)

Resolves #610

Description

This PR add an object marker to stored documents. This helps the iterator return an empty documents.

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

fredcarle · 2022-09-15T08:16:05Z

core/key.go

+	if len(elements) == 6 {
+		return DataStoreKey{
+			CollectionId: elements[3],
+			InstanceType: InstanceType(elements[4]),
+			DocKey:       elements[5],
+		}
+	}
+
+	numberOfElements := len(elements)


@AndrewSisley you may not like this so let me know if you think I should do something different.

Yeah, I would suggest adding a new Key type for this - it doesn't use all the props and you'll be able to give it a clearer name/docs.

EDIT: Although conceptually this can perhaps be seen as nil field, and breaking it out to a new type would make the fetcher code more complex? Suggest some docs in here if you keep like this

If we were using a new key type, it would end up being a duplicate of the primary key in a sense. This means that we could instead use the primary key to accomplish this but that would make the fetcher more complex.

Not fussed about the duplication (is a coincidence), but whether it is worth the extra fetcher hassle is quite tenuous

shahzadlone · 2022-09-15T08:22:49Z

db/fetcher/fetcher.go

-		// skip object markers
-		if bytes.Equal(df.kv.Value, []byte{base.ObjectMarker}) {
-			continue
-		}
-


suggestion: You don't need the for loop anymore, as all branches now return according to logic. Is that desired? if so then remove the outer for { }

indeed. I had removed it initially and then reverted while trying to find a problem and never re-removed it.

AndrewSisley

Really surprised how little code you managed to do this in :) And I was wondering if we'd have to go this route :( Got a todo for you regarding extra scan rows for docs with fields - that one is fairly significant, and a minor skipable comment RE to dockey lines.

AndrewSisley · 2022-09-15T15:16:42Z

core/key.go

+	if len(elements) == 6 {
+		return DataStoreKey{
+			CollectionId: elements[3],
+			InstanceType: InstanceType(elements[4]),
+			DocKey:       elements[5],
+		}
+	}
+
+	numberOfElements := len(elements)


Yeah, I would suggest adding a new Key type for this - it doesn't use all the props and you'll be able to give it a clearer name/docs.

EDIT: Although conceptually this can perhaps be seen as nil field, and breaking it out to a new type would make the fetcher code more complex? Suggest some docs in here if you keep like this

AndrewSisley · 2022-09-15T15:19:11Z

db/collection.go

+
+	// write value object marker
+	valueKey := c.getDatastoreFromDocKey(dockey)
+	err = txn.Datastore().Put(ctx, valueKey.ToDS(), []byte{base.ObjectMarker})


todo: Would really suggest only adding these if the doc is empty/nil, otherwise you are adding an extra scan row to every doc in the db. We got a notable performance gain breaking out the PKs.

codecov · 2022-09-15T16:10:06Z

Codecov Report

Merging #800 (ce9fd09) into develop (57a9394) will increase coverage by 0.00%.
The diff coverage is 69.23%.

@@           Coverage Diff            @@
##           develop     #800   +/-   ##
========================================
  Coverage    59.53%   59.54%           
========================================
  Files          154      154           
  Lines        17181    17219   +38     
========================================
+ Hits         10229    10253   +24     
- Misses        6030     6040   +10     
- Partials       922      926    +4

Impacted Files	Coverage Δ
core/key.go	`85.24% <62.50%> (-2.54%)`	⬇️
db/collection.go	`65.92% <66.66%> (+0.02%)`	⬆️
db/fetcher/fetcher.go	`61.06% <78.26%> (+0.36%)`	⬆️
connor/lt.go	`72.72% <0.00%> (-4.55%)`	⬇️

AndrewSisley · 2022-09-15T16:12:15Z

core/key.go

+	if isDataObjectMarker(elements) {
+		return DataStoreKey{
+			CollectionId: elements[3],
+			InstanceType: InstanceType(elements[4]),


suggestion(non-bocking): IMO it would be clearer to just hardcode InstanceType here, as the current suggests that it can be stuff other than Value

AndrewSisley

Good job :) Hope you had fun figuring this out :D

) This PR add an object marker to stored documents. This helps the iterator return an empty documents.

fredcarle added bug Something isn't working area/query Related to the query component action/no-benchmark Skips the action that runs the benchmark. labels Sep 15, 2022

fredcarle added this to the DefraDB v0.3.1 milestone Sep 15, 2022

fredcarle requested a review from a team September 15, 2022 08:14

fredcarle self-assigned this Sep 15, 2022

fredcarle commented Sep 15, 2022

View reviewed changes

shahzadlone requested changes Sep 15, 2022

View reviewed changes

AndrewSisley requested changes Sep 15, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/fix/empty-doc branch from 0edc20d to 93cae66 Compare September 15, 2022 16:05

fredcarle requested review from shahzadlone and AndrewSisley September 15, 2022 16:07

AndrewSisley reviewed Sep 15, 2022

View reviewed changes

AndrewSisley approved these changes Sep 15, 2022

View reviewed changes

fredcarle added 3 commits September 15, 2022 15:47

add object marker to return empty docs

279543d

apply feedback

8ffafe3

add documentation of the change to the data-structure

2eede48

fredcarle force-pushed the fredcarle/fix/empty-doc branch from d5e8514 to 2eede48 Compare September 15, 2022 19:47

change to hardcoded instance type

ce9fd09

shahzadlone approved these changes Sep 15, 2022

View reviewed changes

fredcarle merged commit 39033f1 into develop Sep 15, 2022

fredcarle deleted the fredcarle/fix/empty-doc branch September 15, 2022 20:01

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024

fix: Add object marker to enable return of empty docs (sourcenetwork#800

5f70fd4

) This PR add an object marker to stored documents. This helps the iterator return an empty documents.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add object marker to enable return of empty docs #800

fix: Add object marker to enable return of empty docs #800

fredcarle commented Sep 15, 2022

fredcarle Sep 15, 2022

AndrewSisley Sep 15, 2022

fredcarle Sep 15, 2022

AndrewSisley Sep 15, 2022

shahzadlone Sep 15, 2022

fredcarle Sep 15, 2022

AndrewSisley left a comment

AndrewSisley Sep 15, 2022

AndrewSisley Sep 15, 2022

codecov bot commented Sep 15, 2022 •

edited

Loading

AndrewSisley Sep 15, 2022

AndrewSisley left a comment

fix: Add object marker to enable return of empty docs #800

fix: Add object marker to enable return of empty docs #800

Conversation

fredcarle commented Sep 15, 2022

Relevant issue(s)

Description

Tasks

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 15, 2022 •

edited

Loading