fix: Allow querying of 9th, 19th, 29th, etc collections #2819

AndrewSisley · 2024-07-08T20:07:19Z

Relevant issue(s)

Resolves #2810

Description

Allows querying of 9th, 19th, 29th, etc collections.

The fetcher calls PrefixEnd which causes the start prefix to be (e.g.) "/9" and the end "/10" causing nothing to be returned to the user. This PR fixes that by encoding the DataStore CollectionRootIDs as variable sized BigEndians.

#2818 has been broken out of this as it is not a user-visible issue atm and resolving it is a bit more involved due to some legacy tech. debt.

codecov · 2024-07-08T20:18:12Z

Codecov Report

Attention: Patch coverage is 92.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.14%. Comparing base (24fa14f) to head (0afbc3c).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2819      +/-   ##
===========================================
+ Coverage    78.60%   79.14%   +0.54%     
===========================================
  Files          318      318              
  Lines        24128    24162      +34     
===========================================
+ Hits         18965    19123     +158     
+ Misses        3785     3662     -123     
+ Partials      1378     1377       -1

Flag	Coverage Δ
all-tests	`79.14% <92.86%> (+0.54%)`	⬆️

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

Files	Coverage Δ
internal/core/key.go	`83.96% <100.00%> (-0.03%)`	⬇️
internal/planner/scan.go	`90.75% <100.00%> (+1.16%)`	⬆️
internal/core/encoding.go	`79.63% <91.49%> (+3.30%)`	⬆️

... and 25 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 24fa14f...0afbc3c. Read the comment docs.

shahzadlone · 2024-07-08T20:33:23Z

tests/integration/issues/2810_test.go

@@ -0,0 +1,315 @@
+// Copyright 2024 Democratized Data Foundation


Praise: Thanks a bunch for documenting these! Really nice to see the issue narrowed down with the edge cases you highlighted.

shahzadlone · 2024-07-08T20:41:21Z

docs/website/guides/explain-systems.md

@@ -101,8 +101,8 @@ query @explain {
                    "collectionID": "3",
                    "collectionName": "Author",
                    "spans": [{
-                        "start": "/3",
-                        "end": "/4"
+                        "start": "/\x8b",


nitpick: My eyes 😮

Has been reverted - encoding is not json friendly, so explain must be pretty-printed anyway

shahzadlone · 2024-07-08T20:44:28Z

internal/core/key_test.go

@@ -34,86 +34,6 @@ func TestNewDataStoreKey_ReturnsEmptyStruct_GivenEmptyString(t *testing.T) {
 	assert.ErrorIs(t, ErrEmptyKey, err)
 }

-func TestNewDataStoreKey_ReturnsCollectionIdAndIndexIdAndDocIDAndFieldIdAndInstanceType_GivenFourItemsWithType(


question: Do we loose any coverage or paths that these tests were testing due to the removal?

I doubt it, in theory everything should be covered by integration tests anyway, and the new code has fewer branches IIRC.

shahzadlone · 2024-07-08T20:46:24Z

internal/core/encoding.go

+	}, nil
+}
+
+func EncodeDataStoreKey(key *DataStoreKey) []byte {


suggestion: A line documenting the Encoding type

Will add

doc func

shahzadlone · 2024-07-08T20:46:40Z

internal/core/encoding.go

@@ -288,3 +288,81 @@ func EncodeIndexDataStoreKey(key *IndexDataStoreKey) []byte {

 	return b
 }
+
+func DecodeDataStoreKey(data []byte) (DataStoreKey, error) {


suggestion: A line documenting the decoding type

Will add

doc func

shahzadlone

Left some comments

islamaliev

Looks good. I left some clarifying comments.

islamaliev · 2024-07-08T20:09:07Z

tests/integration/utils2.go

@@ -1301,6 +1300,7 @@ func createDocViaGQL(
 	node client.DB,
 	collections []client.Collection,
 ) ([]*client.Document, error) {
+	println(fmt.Sprint(collections))


question: is it left here intentional?

It is not :) And I should have been more careful so soon after the mutation type bug :) Thanks for spotting

remove print

islamaliev · 2024-07-08T20:30:15Z

internal/core/encoding.go

+	}
+
+	var docID string
+	if len(data) > 40 {


todo: please extract the magic number to a const

Will do

Rm magic number

islamaliev · 2024-07-08T20:33:27Z

internal/core/encoding.go

+	}
+
+	var instanceType InstanceType
+	if len(data) > 1 {


suggestion: wouldn't it be better to do:

if len(data) <= 1 { return ... } // check instance type ...

Currently if if len(data) > 1 is false the rest of the function is irrelevant, but is being executed.

I do prefer the simplicity of the current code, although it does have a small runtime cost. I'll have a quick look when I remove the magic number - do let me know if you have a strong preference though.

Have quick look at early returns

I wouldn't call it a strong preference. Just following return-early best practice.

I had a play, whilst I like it hightlights the data flow a little more nicely, it ends up duplicating logic (or mutating, or a new func) and even without my gut prefers the feel of the current. Leaving as-is.

islamaliev · 2024-07-08T20:42:21Z

tests/integration/explain/default/top_with_count_test.go

-									"start": "/3",
-									"end":   "/4",
+									"start": "/\x8b",
+									"end":   "/\x8c",


though: this is pain. Would be nice to have it extracted so that if it changes again, we don't need to go through it again.

Explain was actually pretty easy considering how invasive they are by nature, the unit tests were much more of a pain.

This might get changed slightly before merge anyway - the http client does not like this encoding and it caught me out.

This has been reverted, the encoding was not json friendly and needed to be prettied up in order to work with the http client.

islamaliev · 2024-07-08T20:45:12Z

tests/integration/query/simple/with_multiple_types_test.go

+	testUtils "github.com/sourcenetwork/defradb/tests/integration"
+)
+
+func TestSimple_WithSevenDummyTypesBefore(t *testing.T) {


nitpick: would be nice to have "expectation" part in the test name:
TestSimple_WithSevenDummyTypesBefore_ShouldSucceed or something like this.

islamaliev · 2024-07-08T20:47:19Z

tests/integration/query/simple/with_multiple_types_test.go

+					type Type6 {
+						f: String
+					}
+
+					type User {
+						name: String
+					}


suggestion: for these tests would be nice to have some more information (as a documentation or test description) explaining why these weird-looking tests are necessary and which edge-case they test.

Agreed, I'll add some in - thanks for pushing past my laziness here :)

Add test docs

islamaliev · 2024-07-08T20:48:15Z

internal/core/key_test.go

-func TestNewDataStoreKey_GivenAStringWithExtraSuffix(t *testing.T) {
-	instanceType := "anyType"
-	fieldId := "f1"
-	docID := "docID"
-	collectionId := "1"
-	inputString := "/db/data/" + collectionId + "/" + instanceType + "/" + docID + "/" + fieldId + "/version_number"
-
-	_, err := NewDataStoreKey(inputString)
-
-	assert.ErrorIs(t, ErrInvalidKey, err)
-}


question: you just removed the tests. Don't you plan to replace them with new ones?

No, I don't think they had much value before, and even less so with the new code IMO.

shahzadlone

Please double check when all tests are passing that the removed unit tests weren't testing any edge cases before merge, other than that all good! Thanks a bunch for sorting this out.

Broken out of prod code commit to not clutter primary changes.

…k#2819) ## Relevant issue(s) Resolves sourcenetwork#2810 ## Description Allows querying of 9th, 19th, 29th, etc collections. The fetcher calls `PrefixEnd` which causes the start prefix to be (e.g.) `"/9"` and the end `"/10"` causing nothing to be returned to the user. This PR fixes that by encoding the DataStore CollectionRootIDs as variable sized BigEndians. sourcenetwork#2818 has been broken out of this as it is not a user-visible issue atm and resolving it is a bit more involved due to some legacy tech. debt.

AndrewSisley added bug Something isn't working area/datastore Related to the datastore / storage engine system labels Jul 8, 2024

AndrewSisley added this to the DefraDB v0.13 milestone Jul 8, 2024

AndrewSisley requested a review from a team July 8, 2024 20:07

AndrewSisley self-assigned this Jul 8, 2024

AndrewSisley added 2 commits July 8, 2024 16:24

Remove hardcoded gql type

45089ac

Add tests documenting issue

8fdc7f4

AndrewSisley force-pushed the 2810-dave-many-many-bug branch from e00ddf8 to fdeb99b Compare July 8, 2024 20:24

shahzadlone reviewed Jul 8, 2024

View reviewed changes

Encode col root id as big-endian bytes

6c0d881

AndrewSisley force-pushed the 2810-dave-many-many-bug branch from fdeb99b to a62566a Compare July 8, 2024 20:33

shahzadlone reviewed Jul 8, 2024

View reviewed changes

islamaliev approved these changes Jul 8, 2024

View reviewed changes

shahzadlone approved these changes Jul 8, 2024

View reviewed changes

AndrewSisley force-pushed the 2810-dave-many-many-bug branch from a62566a to 8d69938 Compare July 8, 2024 21:30

AndrewSisley added 3 commits July 8, 2024 17:30

Pretty print explain keys

be2856c

FIXUP - core unit tests

4c450c0

Broken out of prod code commit to not clutter primary changes.

Remove old datastore key unit tests

0488b30

AndrewSisley force-pushed the 2810-dave-many-many-bug branch from 8d69938 to 0488b30 Compare July 8, 2024 21:30

AndrewSisley added 5 commits July 8, 2024 17:35

PR FIXUP - Document encoding funcs

b0fef1d

PR FIXUP - Remove accidental print

3529335

PR FIXUP - Remove magic number

6a9f7d9

PR FIXUP - Document new tests

9a5d274

PR FIXUP - Document breaking change

0afbc3c

AndrewSisley merged commit 1acc084 into sourcenetwork:develop Jul 9, 2024
39 checks passed

AndrewSisley deleted the 2810-dave-many-many-bug branch July 9, 2024 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Allow querying of 9th, 19th, 29th, etc collections #2819

fix: Allow querying of 9th, 19th, 29th, etc collections #2819

AndrewSisley commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

shahzadlone Jul 8, 2024

shahzadlone Jul 8, 2024

AndrewSisley Jul 8, 2024

shahzadlone Jul 8, 2024

AndrewSisley Jul 8, 2024

shahzadlone Jul 8, 2024

AndrewSisley Jul 8, 2024 •

edited

Loading

shahzadlone Jul 8, 2024

AndrewSisley Jul 8, 2024 •

edited

Loading

shahzadlone left a comment

islamaliev left a comment

islamaliev Jul 8, 2024

AndrewSisley Jul 8, 2024 •

edited

Loading

islamaliev Jul 8, 2024

AndrewSisley Jul 8, 2024 •

edited

Loading

islamaliev Jul 8, 2024

AndrewSisley Jul 8, 2024 •

edited

Loading

islamaliev Jul 8, 2024

AndrewSisley Jul 8, 2024

islamaliev Jul 8, 2024

AndrewSisley Jul 8, 2024

AndrewSisley Jul 8, 2024

islamaliev Jul 8, 2024

islamaliev Jul 8, 2024

AndrewSisley Jul 8, 2024 •

edited

Loading

islamaliev Jul 8, 2024

AndrewSisley Jul 8, 2024

shahzadlone left a comment

		@@ -0,0 +1,315 @@
		// Copyright 2024 Democratized Data Foundation

fix: Allow querying of 9th, 19th, 29th, etc collections #2819

fix: Allow querying of 9th, 19th, 29th, etc collections #2819

Conversation

AndrewSisley commented Jul 8, 2024 • edited Loading

Relevant issue(s)

Description

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

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 Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 8, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

AndrewSisley commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

AndrewSisley Jul 8, 2024 •

edited

Loading

AndrewSisley Jul 8, 2024 •

edited

Loading

AndrewSisley Jul 8, 2024 •

edited

Loading

AndrewSisley Jul 8, 2024 •

edited

Loading

AndrewSisley Jul 8, 2024 •

edited

Loading

AndrewSisley Jul 8, 2024 •

edited

Loading