-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(torii-grpc): building sql query for array idx #2593
Conversation
WalkthroughOhayo, sensei! The changes in this pull request primarily focus on enhancing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModelSQLReader
participant SQLite
User->>ModelSQLReader: Request SQL Query
ModelSQLReader->>SQLite: Build SQL Query
SQLite-->>ModelSQLReader: Return SQL Query
ModelSQLReader->>SQLite: Execute Query
SQLite-->>ModelSQLReader: Return Results
ModelSQLReader->>User: Return Results
🪧 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/core/src/model.rs (1)
419-421
: Consider optimizing join performance
The current implementation might benefit from index hints for the join conditions, especially for the full_array_id
column.
Consider adding an index on full_array_id
if not already present, and potentially using index hints in the join clauses for better query performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/torii/core/src/model.rs (1 hunks)
🔇 Additional comments (2)
crates/torii/core/src/model.rs (2)
419-421
: Ohayo, sensei! The join clause construction looks good!
The new enumeration-based approach correctly differentiates between the first table (using entity_id) and subsequent tables (using full_array_id) in the join sequence. This fix properly handles array indexing in SQL queries.
419-421
: 🛠️ Refactor suggestion
Consider adding error handling for array indices
While the join clause construction is correct, it might be worth adding validation for array indices to prevent potential out-of-bounds scenarios.
Let's verify the array handling in the codebase:
Consider adding bounds checking:
.enumerate()
.map(|(i, table)| {
+ if table.table_name.is_empty() {
+ return Err(QueryError::InvalidTableName);
+ }
if i == 0 {
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2593 +/- ##
==========================================
+ Coverage 69.65% 69.66% +0.01%
==========================================
Files 401 401
Lines 50818 50819 +1
==========================================
+ Hits 35395 35404 +9
+ Misses 15423 15415 -8 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Bug Fixes
Refactor