feat: Sec. index on json #3330
feat: Sec. index on json #3330
Conversation
islamaliev
added
area/query
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3330 +/- ##
===========================================
+ Coverage 78.05% 78.20% +0.15%
===========================================
Files 388 391 +3
Lines 35398 35602 +204
===========================================
+ Hits 27629 27842 +213
+ Misses 6133 6126 -7
+ Partials 1636 1634 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
A few comments to start. There are a couple areas I want to go over again but in general I would say this PR looks really good. The JSON index feature is really powerful and will certainly be a selling point for a lof of devs that need to handle JSON metadata.
| // JSONVisitor is a function that processes a JSON value at a given path. | ||
| // path represents the location of the value in the JSON tree. |
There was a problem hiding this comment.
question: There is no path in the function signature. Was this forgotten from a previous implementation?
edit: I see that it's in reference to the JSON parameter that has the path internally. Maybe just rephrase so that it reflect that instead. at a given path makes the reader believe that a path parameter should be provided and that seems like a mistake.
There was a problem hiding this comment.
praise: Thank you for writing this documentation. It's very informative and will be useful both to help write the external documentation and for future devs working on indexes.
| if cond.op == compOpAny || cond.op == compOpAll || cond.op == compOpNone { | ||
| subCondMap := filterVal.(map[connor.FilterKey]any) | ||
| for subKey, subVal := range subCondMap { | ||
| // TODO: check what happens with _any: {_eq: [1, 2]} |
There was a problem hiding this comment.
todo: I assume this should be done before merging.
| @@ -691,7 +459,15 @@ func (f *IndexFetcher) createIndexIterator() (indexIterator, error) { | |||
| } else if fieldConditions[0].op == opIn && fieldConditions[0].arrOp != compOpNone { | |||
| iter, err = f.newInIndexIterator(fieldConditions, matchers) | |||
| } else { | |||
| iter, err = f.newPrefixIterator(f.newIndexDataStoreKey(), matchers, &f.execInfo), nil | |||
| key := f.newIndexDataStoreKey() | |||
| // TODO: can we test fieldConditions[not 0]? | |||
There was a problem hiding this comment.
todo: Please resolve this or create an issue for it.
| fieldsDescs []client.SchemaFieldDescription | ||
| collection client.Collection | ||
| desc client.IndexDescription | ||
| // fieldsDescs is a slice of field descriptions for the fields that are indexed by the index |
There was a problem hiding this comment.
suggestion: "for the fields that form the index"
| @@ -186,6 +241,47 @@ func (index *collectionBaseIndex) Description() client.IndexDescription { | |||
| return index.desc | |||
| } | |||
|
|
|||
| func (index *collectionBaseIndex) generateIndexKeys( | |||
There was a problem hiding this comment.
suggestion: The name of this method and the one ForField version are both misleading as they don't just generate index keys. The function f is a side effect that brings a bit of confusion when reading the code. Like the name suggests that we want to generate index keys but no keys are returned. I know this is internal but what is happening is so not obvious that documentation would be very helpful.
| b = append(b, jsonMarker) | ||
| for _, part := range v.GetPath() { | ||
| pathBytes := unsafeConvertStringToBytes(part) | ||
| //b = encodeBytesAscendingWithTerminator(b, pathBytes, ascendingBytesEscapes.escapedTerm) |
There was a problem hiding this comment.
todo: Remove commented line.
| @@ -1313,7 +1313,7 @@ func TestQueryWithUniqueCompositeIndex_AfterUpdateOnNilFields_ShouldFetch(t *tes | |||
| }, | |||
| }, | |||
| }, | |||
| testUtils.Request{ | |||
| /*testUtils.Request{ | |||
There was a problem hiding this comment.
todo: Uncomment or remove commented code.
Relevant issue(s)
Resolves #2280
Description
Enables json fields indexing.
JSON interface has been extended to allow traversing it with different configurations.
Indexing or documents has been refactor so that instead of acting based off of the fact that there is a special field in the index description (like array or json), we assign a field-specific generator so that every field is responsible for generating a value for the inde key. For example, if we have a composite index made up of fields of types int, array and json (complex composite index), we will generate all possible combinations where int generator will always generate 1 value, array generator will generate values for every element and json generator will generate values for every json node.
Added json encoding/decoding to our encoding package.