feat: Add composite indexes #2226
Conversation
islamaliev
added
feature
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2226 +/- ##
===========================================
- Coverage 74.08% 0 -74.08%
===========================================
Files 256 0 -256
Lines 25627 0 -25627
===========================================
- Hits 18985 0 -18985
+ Misses 5333 0 -5333
+ Partials 1309 0 -1309
Flags with carried forward coverage won't be shown. Click here to find out more. see 256 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -185,7 +185,12 @@ func normalizeProperties(parentKey connor.FilterKey, conditions []any) []any { | |||
| // if canMergeAnd is true, all _and groups will be merged | |||
| props := make(map[int][]any) | |||
| for _, c := range conditions { | |||
| for key, val := range c.(map[connor.FilterKey]any) { | |||
| cMap, ok := c.(map[connor.FilterKey]any) | |||
There was a problem hiding this comment.
question: Why has this changed? What else could it be?
There was a problem hiding this comment.
normalizeProperty method that calls this normalizeProperties checks is given condition is map[connor.FilterKey]any or an array of conditions which has a type of []any. But as it recursively checks all nested conditions and values one of the values (not conditions) can match []any (for example "_in" op). That's why we need to check if it's really a conditions or just value.
There was a problem hiding this comment.
Ah okay - this was a bug fix? Did you add an integration test that covers this?
There was a problem hiding this comment.
an integration test found this bug :)
tests/integration/index/query_with_composite_index_only_filter_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good to me, thanks Islam :)
I'm approving now, as all but one of my remaining comments are very optional within this PR. I do think we need the following though before merge please:
Can you please make sure that there are integration tests that make sure that if there are multiple overlapping indexes (e.g. two composites, a simple and a composite) that queries still work (no need for explain test documenting this issue, but that would be nice if you can do it reliably without much effort too).
And preferably at least one for what looks like a bug fix in filter/normailze.go (if it is a bug fix :))
islamaliev
force-pushed
the
feat/composite-indexes
branch
from
02bf8c8 to
fbb69d2
Compare
## Relevant issue(s) Resolves #299 ## Description This change introduces composite secondary indexes as well as unique-composite secondary indexes. Note: different order direction for composite fields is not included in this change.
## Relevant issue(s) Resolves sourcenetwork#299 ## Description This change introduces composite secondary indexes as well as unique-composite secondary indexes. Note: different order direction for composite fields is not included in this change.
## Relevant issue(s) Resolves sourcenetwork#299 ## Description This change introduces composite secondary indexes as well as unique-composite secondary indexes. Note: different order direction for composite fields is not included in this change.
## Relevant issue(s) Resolves sourcenetwork#299 ## Description This change introduces composite secondary indexes as well as unique-composite secondary indexes. Note: different order direction for composite fields is not included in this change.
Relevant issue(s)
Resolves #299
Description
This change introduces composite secondary indexes as well as unique-composite secondary indexes.
Note: different order direction for composite fields is not included in this change.
Tasks
How has this been tested?
Unit and integration tests.
Specify the platform(s) on which this was tested: