fix: keys composite clause #2798
Conversation
WalkthroughOhayo, sensei! This pull request introduces modifications across several files, primarily enhancing the functionality of the Changes
Possibly related PRs
Suggested reviewers
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
|
![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 and nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
1165-1176: Ohayo, sensei! Consider optimizing model checks in composite clausesThe current implementation uses
group_concatandINSTRto check for the presence of model IDs inhaving_clauses. For large datasets, this might impact performance. Consider usingEXISTSor additionalJOINclauses to improve query efficiency when filtering by models.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(2 hunks)
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
1127-1127: Ohayo, sensei! The regex pattern correctly handles variable-length keys
The addition of "/({})*" to the keys_pattern ensures that when pattern_matching is set to VariableLen, the regex will match variable-length keys as intended.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2798 +/- ##
==========================================
- Coverage 55.75% 55.74% -0.02%
==========================================
Files 439 439
Lines 55522 55544 +22
==========================================
+ Hits 30957 30962 +5
- Misses 24565 24582 +17 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
1177-1179: Consider enhancing the error message for invalid model format.The error message for invalid namespaced models could be more descriptive to help users understand the expected format.
- .ok_or(QueryError::InvalidNamespacedModel(model.clone()))?; + .ok_or_else(|| QueryError::InvalidNamespacedModel( + format!("Invalid model format '{}'. Expected format: 'namespace-model'", model) + ))?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(3 hunks)
🔇 Additional comments (3)
crates/torii/grpc/src/server/mod.rs (3)
745-763: Ohayo sensei! Clean and well-structured SQL query construction.
The SQL query construction logic elegantly handles both having and non-having clause cases, with proper table aliasing and efficient counting strategies.
1136-1136: Ohayo sensei! Proper path separator handling in regex pattern.
The addition of the forward slash before the KEY_PATTERN ensures correct path separation in variable length matching scenarios.
1174-1186: Ohayo sensei! Solid implementation of model checks in Keys clause.
The addition of model checks through INSTR conditions in the having clause ensures proper validation of model existence.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/torii/src/main.rs(1 hunks)crates/torii/cli/src/options.rs(3 hunks)crates/torii/core/src/engine.rs(3 hunks)crates/torii/grpc/src/server/mod.rs(7 hunks)eternum.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternum.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/grpc/src/server/mod.rs
🔇 Additional comments (5)
bin/torii/src/main.rs (1)
167-167: Ohayo! LGTM - Clean integration of world_block configuration.
The change correctly propagates the world block configuration from CLI arguments to the engine configuration.
crates/torii/cli/src/options.rs (2)
160-172: Ohayo! Clean implementation of world_block configuration.
The implementation is well-documented and includes proper default values.
Also applies to: 186-186
226-228: LGTM - Proper merge behavior.
The merge function correctly updates world_block only when the current value is 0, maintaining proper override behavior.
crates/torii/core/src/engine.rs (2)
151-151: Ohayo! Clean addition of world_block to engine configuration.
The configuration and default implementation are consistent with the CLI options.
Also applies to: 163-163
328-328:
Verify the indexing behavior with non-zero world_block.
The change modifies the starting point for data fetching. This is a significant change that could affect the consistency of indexed data.
Consider adding validation to ensure that setting a non-zero world_block doesn't break data consistency, especially for:
- State that depends on sequential block processing
- Cumulative calculations that require complete history
- Token balances that need accurate historical data
crates/torii/cli/src/options.rs
Outdated
| /// Warning: In the current implementation, this will break the indexing of tokens, if any. | ||
| /// Since the tokens require the chain to be indexed from the beginning, to ensure correct | ||
| /// balance updates. |
There was a problem hiding this comment.
💡 Codebase verification
Warning: Token indexing requires sequential block processing, sensei!
Based on the search results and the warning comment, starting indexing from a non-zero block can indeed break token balance tracking. This is because token balance updates need to be processed sequentially from the beginning to maintain accurate state.
Key points:
- Token indexing requires processing from genesis to maintain correct balance updates
- Setting a non-zero world_block will skip earlier transactions that may affect token balances
- This can lead to incorrect token balance states
🔗 Analysis chain
Verify the impact on token indexing, sensei!
The documentation warns about potential issues with token indexing when setting a non-zero world_block. Let's verify this limitation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for token-related indexing logic that might be affected
# Look for token balance update logic
rg -A 5 "balance.*update"
# Look for token transfer event processors
ast-grep --pattern 'class $_ {
$$$
process($_, $_, $block_number, $_, $_, $_, $_) {
$$$
}
$$$
}'
Length of output: 66855
Script:
#!/bin/bash
# Let's check for token balance indexing logic and block number dependencies
# Search for token balance updates with block number context
rg -A 5 "balance.*block.*number"
# Look for token indexing or state update logic
rg -A 5 "token.*index|state.*update"
# Search for block number validation or requirements
rg -A 5 "block.*require|block.*valid"
# Check for any token-related storage operations
rg -A 5 "Storage.*token|token.*storage"
Length of output: 65691
Script:
#!/bin/bash
# Let's check for token indexing and block number dependencies in the codebase
# Search for token indexing implementation
rg -A 5 "index.*token|token.*index"
# Look for block number usage in token-related code
rg -A 5 "block.*number.*token|token.*block"
# Check for any token state management code
rg -A 5 "token.*state|state.*token"
# Search specifically in the torii codebase for token indexing
fd -t f . crates/torii | xargs rg -A 5 "token.*index|index.*token"
Length of output: 69333
Larkooo
force-pushed
the
fix-composite
branch
from
90da3be to
22a2f8e
Compare
* fix: keys composite clause * count * c * f
Summary by CodeRabbit
New Features
Bug Fixes