feat(collections): support indexing #20704
Conversation
WalkthroughWalkthroughThe changes introduce enhancements to the collections framework, focusing on schema indexing capabilities. Key updates include the addition of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SchemaManager
participant Codec
participant Collection
User->>SchemaManager: Request schema codec
SchemaManager->>Codec: Fetch schema codec
Codec-->>SchemaManager: Return SchemaCodec
SchemaManager->>Collection: Apply schema changes
Collection-->>User: Confirm schema update
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…er-base-validation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
collections/CHANGELOG.md (1)
41-41: Simplify changelog entry wording.Consider simplifying the wording for brevity.
- Add `ModuleCodec` method to `Schema` and `HasSchemaCodec` interface in order to support `cosmossdk.io/schema` compatible indexing. + Add `ModuleCodec` method to `Schema` and `HasSchemaCodec` interface to support `cosmossdk.io/schema` compatible indexing.Tools
LanguageTool
[style] ~41-~41: Consider a shorter alternative to avoid wordiness.
Context: ...SchemaandHasSchemaCodecinterface in order to supportcosmossdk.io/schemacompatibl...(IN_ORDER_TO_PREMIUM)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- collections/CHANGELOG.md (1 hunks)
- collections/go.mod (1 hunks)
Additional context used
Path-based instructions (1)
collections/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
collections/CHANGELOG.md
[style] ~41-~41: Consider a shorter alternative to avoid wordiness.
Context: ...SchemaandHasSchemaCodecinterface in order to supportcosmossdk.io/schemacompatibl...(IN_ORDER_TO_PREMIUM)
Additional comments not posted (2)
collections/go.mod (2)
23-23: Verify local replacement directive.The replacement of
cosmossdk.io/schemawith a local path../schemasuggests local development or testing. Ensure this change is intentional and correctly set up for local development.
8-8: Verify new dependencies.Ensure that adding
cosmossdk.io/schema v0.1.0andjackfan.us.kg/tidwall/btree v1.7.0as direct dependencies is intentional and does not introduce conflicts or issues in the codebase.Also applies to: 10-10
Verification successful
Dependencies are intentionally added and used.
The dependencies
cosmossdk.io/schema v0.1.0andjackfan.us.kg/tidwall/btree v1.7.0are intentionally added to thego.modfile and are actively used in the codebase without any apparent conflicts.
cosmossdk.io/schemais used in multiple files such asschema/view/object.goandindexer/postgres/internal/testdata/example_schema.go.github.com/tidwall/btreeis used extensively across various files, indicating its integration into the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of new dependencies in the codebase. # Test: Search for imports of the new dependencies. Expect: Relevant usage in the codebase. rg --type go 'import.*(cosmossdk\.io/schema|github\.com/tidwall/btree)'Length of output: 257
Script:
#!/bin/bash # Check if github.com/tidwall/btree is listed in the go.mod file rg 'github\.com/tidwall/btree' collections/go.mod # Search for any usage of btree in the codebase rg --type go 'btree'Length of output: 5105
| // isSecondaryIndex indicates that this map represents a secondary index | ||
| // on another collection and that it should be skipped when generating | ||
| // a user facing schema | ||
| isSecondaryIndex bool |
There was a problem hiding this comment.
q: could it be useful to still generate this optionally? Or is it absolutely unnecessary? (asking out of ignorance)
There was a problem hiding this comment.
The target database (ex. postgres) should generally be able to produce any secondary index it wants if the primary data is present. So this isn't needed.
Co-authored-by: Facundo Medica <[email protected]>
Co-authored-by: Facundo Medica <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- collections/codec/indexing.go (1 hunks)
- collections/indexing.go (1 hunks)
- x/accounts/defaults/lockup/go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- x/accounts/defaults/lockup/go.mod
Additional context used
Path-based instructions (2)
collections/codec/indexing.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/indexing.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: tests (03)
collections/indexing.go
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: tests (02)
collections/indexing.go
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: tests (01)
collections/indexing.go
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: tests (00)
collections/indexing.go
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: golangci-lint
collections/indexing.go
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)) (typecheck)
GitHub Check: dependency-review
collections/indexing.go
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
Additional comments not posted (6)
collections/codec/indexing.go (4)
10-22: LGTM!The
HasSchemaCodecinterface is well-defined and follows Go conventions.The code changes are approved.
24-44: LGTM!The
SchemaCodecstruct is well-defined and follows Go conventions. The use offuncfields for conversion is appropriate.The code changes are approved.
46-54: LGTM!The
KeySchemaCodecfunction correctly handles the retrieval of the schema codec, including fallback handling.The code changes are approved.
56-64: LGTM!The
ValueSchemaCodecfunction correctly handles the retrieval of the schema codec, including fallback handling.The code changes are approved.
collections/indexing.go (2)
14-19: LGTM!The
IndexingOptionsstruct is well-defined and follows Go conventions.The code changes are approved.
71-89: LGTM!The
decodeKVfunction is well-structured and handles key-value pair updates correctly.The code changes are approved.
| // FallbackSchemaCodec returns a fallback schema codec for T when one isn't explicitly | ||
| // specified with HasSchemaCodec. It maps all simple types directly to schema kinds | ||
| // and converts everything else to JSON. | ||
| func FallbackSchemaCodec[T any]() SchemaCodec[T] { | ||
| var t T | ||
| kind := schema.KindForGoValue(t) | ||
| if err := kind.Validate(); err == nil { | ||
| return SchemaCodec[T]{ | ||
| Fields: []schema.Field{{ | ||
| // we don't set any name so that this can be set to a good default by the caller | ||
| Name: "", | ||
| Kind: kind, | ||
| }}, | ||
| // these can be nil because T maps directly to a schema value for this kind | ||
| ToSchemaType: nil, | ||
| FromSchemaType: nil, | ||
| } | ||
| } else { | ||
| // we default to encoding everything to JSON | ||
| return SchemaCodec[T]{ | ||
| Fields: []schema.Field{{Kind: schema.JSONKind}}, | ||
| ToSchemaType: func(t T) (any, error) { | ||
| bz, err := json.Marshal(t) | ||
| return json.RawMessage(bz), err | ||
| }, | ||
| FromSchemaType: func(a any) (T, error) { | ||
| var t T | ||
| bz, ok := a.(json.RawMessage) | ||
| if !ok { | ||
| return t, fmt.Errorf("expected json.RawMessage, got %T", a) | ||
| } | ||
| err := json.Unmarshal(bz, &t) | ||
| return t, err | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Address undefined types and methods.
The function provides a comprehensive fallback mechanism. However, the following issues need to be addressed:
schema.Typeis undefined.schema.NewModuleSchemais undefined.update.Removeis undefined inschema.KVPairUpdate.
Ensure these types and methods are correctly defined or imported.
collections/indexing.go
Outdated
| // ModuleCodec returns the ModuleCodec for this schema for the provided options. | ||
| func (s Schema) ModuleCodec(opts IndexingOptions) (schema.ModuleCodec, error) { | ||
| decoder := moduleDecoder{ | ||
| collectionLookup: &btree.Map[string, *collectionSchemaCodec]{}, | ||
| } | ||
|
|
||
| retainDeletions := make(map[string]bool) | ||
| for _, collName := range opts.RetainDeletionsFor { | ||
| retainDeletions[collName] = true | ||
| } | ||
|
|
||
| var types []schema.Type | ||
| for _, collName := range s.collectionsOrdered { | ||
| coll := s.collectionsByName[collName] | ||
|
|
||
| // skip secondary indexes | ||
| if coll.isSecondaryIndex() { | ||
| continue | ||
| } | ||
|
|
||
| cdc, err := coll.schemaCodec() | ||
| if err != nil { | ||
| return schema.ModuleCodec{}, err | ||
| } | ||
|
|
||
| if retainDeletions[coll.GetName()] { | ||
| cdc.objectType.RetainDeletions = true | ||
| } | ||
|
|
||
| types = append(types, cdc.objectType) | ||
|
|
||
| decoder.collectionLookup.Set(string(coll.GetPrefix()), cdc) | ||
| } | ||
|
|
||
| modSchema, err := schema.NewModuleSchema(types...) | ||
| if err != nil { | ||
| return schema.ModuleCodec{}, err | ||
| } | ||
|
|
||
| return schema.ModuleCodec{ | ||
| Schema: modSchema, | ||
| KVDecoder: decoder.decodeKV, | ||
| }, nil |
There was a problem hiding this comment.
Address undefined types and methods.
The function is well-structured and handles options correctly. However, the following issues need to be addressed:
schema.Typeis undefined.schema.NewModuleSchemais undefined.
Ensure these types and methods are correctly defined or imported.
Tools
GitHub Check: tests (03)
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
GitHub Check: tests (02)
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
GitHub Check: tests (01)
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
GitHub Check: tests (00)
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
GitHub Check: golangci-lint
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
GitHub Check: dependency-review
[failure] 32-32:
undefined: schema.Type
[failure] 55-55:
undefined: schema.NewModuleSchema
| func (c collectionSchemaCodec) decodeKVPair(update schema.KVPairUpdate) ([]schema.ObjectUpdate, error) { | ||
| // strip prefix | ||
| key := update.Key | ||
| key = key[len(c.coll.GetPrefix()):] | ||
|
|
||
| k, err := c.keyDecoder(key) | ||
| if err != nil { | ||
| return []schema.ObjectUpdate{ | ||
| {TypeName: c.coll.GetName()}, | ||
| }, err | ||
|
|
||
| } | ||
|
|
||
| if update.Remove { | ||
| return []schema.ObjectUpdate{ | ||
| {TypeName: c.coll.GetName(), Key: k, Delete: true}, | ||
| }, nil | ||
| } | ||
|
|
||
| v, err := c.valueDecoder(update.Value) | ||
| if err != nil { | ||
| return []schema.ObjectUpdate{ | ||
| {TypeName: c.coll.GetName(), Key: k}, | ||
| }, err | ||
| } | ||
|
|
||
| return []schema.ObjectUpdate{ | ||
| {TypeName: c.coll.GetName(), Key: k, Value: v}, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Address undefined field.
The function is well-structured and handles key-value pair updates correctly. However, the field Remove is undefined in schema.KVPairUpdate.
Ensure this field is correctly defined or imported.
Tools
GitHub Check: tests (03)
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: tests (02)
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: tests (01)
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: tests (00)
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
GitHub Check: golangci-lint
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)) (typecheck)
GitHub Check: dependency-review
[failure] 104-104:
update.Remove undefined (type schema.KVPairUpdate has no field or method Remove)
|
Looks like CI is failing here because I'm using a replace for schema in collections, but many places that have collections referenced don't have the replace for schema, but do have a replace for collections. So I think I will wait until I tag schema again to merge this and not use a replace in collections... Marking draft until that's done. |
Pull request was converted to draft
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
collections/go.mod (1)
10-10: Change in dependency status forjackfan.us.kg/tidwall/btree v1.7.0The shift from an indirect to a direct dependency for
github.com/tidwall/btreesuggests a more active use of this package in the codebase, likely related to its utility in indexing operations. This change should be reflected in the documentation to clarify its role and importance in the project.Consider adding documentation or comments in the codebase to explain the specific use cases of
btreein the context of indexing, which will help future developers understand the rationale behind this dependency change.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
collections/go.sumis excluded by!**/*.sum
Files selected for processing (3)
- collections/go.mod (1 hunks)
- collections/indexing.go (1 hunks)
- x/accounts/defaults/lockup/go.mod (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- collections/indexing.go
- x/accounts/defaults/lockup/go.mod
Additional comments not posted (1)
collections/go.mod (1)
8-8: Addition ofcosmossdk.io/schema v0.2.0The addition of this dependency is crucial for the new schema indexing capabilities introduced in this PR. Ensure that the version
0.2.0aligns with the latest stable release that includes all necessary features for schema management.
Description
Introduces basic schema indexing support to collections without any special handling added to codecs. Better naming of fields and better conversion to schema types will be handled in separate PRs (such as #20538). This PR is intended to demonstrate the high level API for schema/indexing support and default handling.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
ModuleCodecmethod in the changelog for improved schema management.Chores