feat(torii-sql): granular model indices #3073
Conversation
|
Ohayo sensei! WalkthroughThe changes extend the Torii application’s command-line interface to support SQL-related options. A new public field is added to the main argument struct to include SQL configuration, and two new structs ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Parser
participant A as ToriiArgs
participant SO as SqlOptions Parser
U->>CLI: Provide command-line input (--sql options)
CLI->>A: Parse arguments (including flattened SQL options)
A->>SO: Parse SQL-specific options via parse_model_indices
SO-->>A: Return parsed SQL configuration
A-->>CLI: Complete configuration with SQL options
Suggested reviewers
Possibly related PRs
✨ Finishing Touches
🪧 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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/cli/src/args.rs(1 hunks)crates/torii/cli/src/options.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/cli/src/options.rs
[error] 385-385: Rust formatting check failed. The help message exceeds the line length limit.
🔇 Additional comments (3)
crates/torii/cli/src/options.rs (3)
375-379: Clean model indices structure with good field naming.Ohayo sensei! The new
ModelIndicesstruct looks well-designed for storing model tags and their associated fields. The field names are clear and self-explanatory.
405-409: Default implementation looks good.The default implementation of
SqlOptionssets sensible defaults -model_indices_keystofalse(meaning all columns get indices by default) andmodel_indicestoNone.
411-423: Solid implementation of the model indices parser.Nice implementation, sensei! The parser correctly handles the expected format and provides a clear error message when the format is invalid.
I particularly like that you're using the
anyhow::Resulttype for error handling, which makes it easy to propagate errors with context.
| #[command(flatten)] | ||
| pub sql: SqlOptions, | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Adding SQL options to the CLI interface.
Ohayo! This change looks good, adding a new sql field to ToriiArgs with the #[command(flatten)] attribute to expose SQL options in the CLI.
However, I noticed that while you've added this field to ToriiArgs, there's no corresponding update to ToriiArgsConfig or the methods that handle configuration file loading (with_config_file and the TryFrom<ToriiArgs> implementation).
Run the following script to examine the current pattern of including options in the configuration structure:
🏁 Script executed:
#!/bin/bash
# Check if other flattened options have config counterparts
rg -A 2 "\[command\(flatten\)\]" crates/torii/cli/src/args.rs
rg "Option<\w+Options>" crates/torii/cli/src/args.rsLength of output: 744
Ohayo, Sensei!
Great job adding the new SQL options to the CLI interface using the flattened attribute. However, our verification shows that unlike the other options (e.g., indexing, events, erc, metrics, server, relay), the sql option lacks a corresponding counterpart in the configuration structure and the associated config file loading methods.
- Please update
ToriiArgsConfigto include an equivalentOption<SqlOptions>field. - Also, revise the configuration file loading methods (like
with_config_fileand theTryFrom<ToriiArgs>implementation) to properly process the new SQL options.
crates/torii/cli/src/options.rs
Outdated
| #[derive(Debug, clap::Args, Clone, Serialize, Deserialize, PartialEq)] | ||
| #[command(next_help_heading = "SQL options")] | ||
| pub struct SqlOptions { | ||
| /// Whether model tables should default to having indices on all columns | ||
| #[arg( | ||
| long = "sql.model_indices_keys", | ||
| default_value_t = false, | ||
| help = "If true, creates indices on only key fields columns of model tables by default. If false, all model field columns will have indices." | ||
| )] | ||
| #[serde(default)] | ||
| pub model_indices_keys: bool, | ||
|
|
||
| /// Specify which fields should have indices for specific models | ||
| /// Format: "model_name:field1,field2;another_model:field3,field4" | ||
| #[arg( | ||
| long = "sql.model_indices", | ||
| value_delimiter = ';', | ||
| value_parser = parse_model_indices, | ||
| help = "Specify which fields should have indices for specific models. Format: \"model_name:field1,field2;another_model:field3,field4\"" | ||
| )] | ||
| #[serde(default)] | ||
| pub model_indices: Option<Vec<ModelIndices>>, | ||
| } |
There was a problem hiding this comment.
Fix the help message that's causing pipeline failure.
The help message for model_indices_keys exceeds the line length limit, causing a CI pipeline failure.
Please break the help message into multiple lines to fix the formatting error:
- help = "If true, creates indices on only key fields columns of model tables by default. If false, all model field columns will have indices."
+ help = "If true, creates indices on only key fields columns of model tables by default. \
+ If false, all model field columns will have indices."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Debug, clap::Args, Clone, Serialize, Deserialize, PartialEq)] | |
| #[command(next_help_heading = "SQL options")] | |
| pub struct SqlOptions { | |
| /// Whether model tables should default to having indices on all columns | |
| #[arg( | |
| long = "sql.model_indices_keys", | |
| default_value_t = false, | |
| help = "If true, creates indices on only key fields columns of model tables by default. If false, all model field columns will have indices." | |
| )] | |
| #[serde(default)] | |
| pub model_indices_keys: bool, | |
| /// Specify which fields should have indices for specific models | |
| /// Format: "model_name:field1,field2;another_model:field3,field4" | |
| #[arg( | |
| long = "sql.model_indices", | |
| value_delimiter = ';', | |
| value_parser = parse_model_indices, | |
| help = "Specify which fields should have indices for specific models. Format: \"model_name:field1,field2;another_model:field3,field4\"" | |
| )] | |
| #[serde(default)] | |
| pub model_indices: Option<Vec<ModelIndices>>, | |
| } | |
| #[derive(Debug, clap::Args, Clone, Serialize, Deserialize, PartialEq)] | |
| #[command(next_help_heading = "SQL options")] | |
| pub struct SqlOptions { | |
| /// Whether model tables should default to having indices on all columns | |
| #[arg( | |
| long = "sql.model_indices_keys", | |
| default_value_t = false, | |
| help = "If true, creates indices on only key fields columns of model tables by default. \ | |
| If false, all model field columns will have indices." | |
| )] | |
| #[serde(default)] | |
| pub model_indices_keys: bool, | |
| /// Specify which fields should have indices for specific models | |
| /// Format: "model_name:field1,field2;another_model:field3,field4" | |
| #[arg( | |
| long = "sql.model_indices", | |
| value_delimiter = ';', | |
| value_parser = parse_model_indices, | |
| help = "Specify which fields should have indices for specific models. Format: \"model_name:field1,field2;another_model:field3,field4\"" | |
| )] | |
| #[serde(default)] | |
| pub model_indices: Option<Vec<ModelIndices>>, | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[error] 385-385: Rust formatting check failed. The help message exceeds the line length limit.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/cli/src/options.rs (1)
375-397:⚠️ Potential issueOhayo sensei, fix the long help message to pass formatting checks.
The pipeline is failing at line 379 for exceeding the Rust style guidelines. Consider splitting the help text across two lines:
- help = "If true, creates indices on only key fields columns of model tables by default. If false, all model field columns will have indices." + help = "If true, creates indices on only key fields columns of model tables by default. \ + If false, all model field columns will have indices."🧰 Tools
🪛 GitHub Actions: ci
[error] 379-379: Rust formatting check failed. Please format the code according to Rust style guidelines.
🧹 Nitpick comments (4)
crates/torii/sqlite/src/types.rs (1)
229-234: Ohayo sensei, consider adding doc comments for the new struct.A short explanation about its purpose and usage can improve maintainability and clarity for other developers.
crates/torii/sqlite/src/lib.rs (3)
42-45: Ohayo sensei, consider documenting SqlConfig.Adding a brief doc comment could clarify the purpose of
model_indicesfor new contributors.
85-91: Ohayo sensei, struct initialization looks neat.Everything is consistent with the newly introduced
configfield. Consider a small doc comment for maintainability if needed.
828-1064: Ohayo sensei, consider splittingadd_columns_recursiveinto smaller methods.This function is quite large and can be broken down for clarity and reduced complexity.
🧰 Tools
🪛 GitHub Actions: ci
[error] 851-851: Rust formatting check failed. Please format the code according to Rust style guidelines.
[error] 867-867: Rust formatting check failed. Please format the code according to Rust style guidelines.
[error] 875-875: Rust formatting check failed. Please format the code according to Rust style guidelines.
[error] 974-974: Rust formatting check failed. Please format the code according to Rust style guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/cli/src/options.rs(2 hunks)crates/torii/runner/src/lib.rs(2 hunks)crates/torii/sqlite/src/lib.rs(6 hunks)crates/torii/sqlite/src/types.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/runner/src/lib.rs
[error] 153-153: Rust formatting check failed. Please format the code according to Rust style guidelines.
crates/torii/cli/src/options.rs
[error] 379-379: Rust formatting check failed. Please format the code according to Rust style guidelines.
crates/torii/sqlite/src/lib.rs
[error] 851-851: Rust formatting check failed. Please format the code according to Rust style guidelines.
[error] 867-867: Rust formatting check failed. Please format the code according to Rust style guidelines.
[error] 875-875: Rust formatting check failed. Please format the code according to Rust style guidelines.
[error] 974-974: Rust formatting check failed. Please format the code according to Rust style guidelines.
🔇 Additional comments (13)
crates/torii/runner/src/lib.rs (2)
41-41: Ohayo sensei, nice new import forSqlConfig.It aligns with the other changes introducing SQL config options. No concerns here.
156-158: Ohayo sensei, verify the silent fallback logic.Using
unwrap_or_default()will silently fallback ifmodel_indicesis not provided in arguments. Confirm whether this is intended or if a warning or explicit error might be more appropriate.crates/torii/cli/src/options.rs (3)
9-9: Ohayo sensei, this import looks correct.
ModelIndicesis indeed used later in this file, so this addition makes sense.
399-403: Ohayo sensei, the default implementation looks good.This ensures consistent behavior whether or not the user provides any SQL options.
405-417: Ohayo sensei, confirm robust parsing.Currently, the parser rejects anything outside the strict "model:field1,field2" format. Consider trimming whitespace or validating empty fields more gracefully.
crates/torii/sqlite/src/lib.rs (8)
24-24: Ohayo sensei, importingModelIndiceshere is consistent.No issues spotted.
53-53: Ohayo sensei, injectingconfigintoSqlis a solid approach.Centralizing SQL-related settings in a dedicated field improves code organization.
69-69: Ohayo sensei, good inclusion ofconfigin the constructor parameters.This ensures it's available throughout the lifecycle of the
Sqlinstance.
784-792: Ohayo sensei, carefully verify the usage ofadd_columns_recursive.You’re passing
upgrade_diffto handle schema changes, so ensure the logic for partial model upgrades is tested thoroughly.
1066-1071: Ohayo sensei, asynchronous execution management is good here.No issues to flag with the
executemethod’s approach.
1072-1076: Ohayo sensei, flush method is straightforward.No concerns—just ensure it’s tested alongside high-concurrency scenarios.
1078-1082: Ohayo sensei, rollback logic looks fine.Ensure any external references are gracefully handled after rollback.
1084-1109: Ohayo sensei, add_controller method is well-designed.Updating or inserting the controller is consistent with the overall pattern used. No immediate concerns.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3073 +/- ##
==========================================
- Coverage 57.39% 57.27% -0.13%
==========================================
Files 440 441 +1
Lines 59963 60223 +260
==========================================
+ Hits 34417 34491 +74
- Misses 25546 25732 +186 ☔ View full report in Codecov by Sentry. |
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 (1)
crates/torii/cli/src/args.rs(10 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/cli/src/args.rs
[error] 253-253: Rust formatting check failed. The code does not adhere to the expected formatting style.
🔇 Additional comments (7)
crates/torii/cli/src/args.rs (7)
54-56: Ohayo! Good job adding the SQL options to the CLI interface.The
sqlfield is properly added to theToriiArgsstruct with the#[command(flatten)]attribute, consistent with how other options are handled.
113-115: Well implemented SQL option merging, Sensei!Your implementation correctly follows the established pattern for merging configuration values from files, checking if the SQL options are set to default values before using the config values.
146-146: Perfect addition to the config structure!This addresses the previous review comment about adding a corresponding
Option<SqlOptions>field to theToriiArgsConfigstruct.
178-178: Consistent handling of SQL options in config conversion!The implementation properly follows the pattern used for other options, only including SQL options in the config if they differ from the default values.
199-199: Good import addition for ModelIndices.Properly imported the
ModelIndicestype from the sqlite module for use in the tests.
221-224: Well-crafted tests for the CLI precedence!The test correctly verifies that SQL options from command line take precedence over those in the config file, and that the model indices are correctly parsed.
Also applies to: 241-243, 256-264
297-300: Comprehensive config fallback test for SQL options!The test properly validates that SQL options from the config file are correctly loaded when no command line options are provided.
Also applies to: 334-340
| assert_eq!(torii_args.events.historical, vec!["a-A".to_string()]); | ||
| assert_eq!(torii_args.server, ServerOptions::default()); | ||
| assert!(torii_args.indexing.transactions); |
There was a problem hiding this comment.
Fix formatting issue flagged by the CI pipeline.
The CI pipeline reports a formatting issue on line 253. Please run cargo fmt to automatically format the code according to Rust's style guidelines.
🧰 Tools
🪛 GitHub Actions: ci
[error] 253-253: Rust formatting check failed. The code does not adhere to the expected formatting style.
|
Creating an index for every column is not a sensible default. If it were, SQlite would do this. Indexes have a trade off and at the extreme of every column has an index, that trade off is almost always -EV. Default should be just the keys. I don't think we need to worry about migrating existing systems, it's fine if this change only applies to new torii instances. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/cli/src/options.rs (2)
394-394: Consider adding a validation note in the help message, senseiThe help message clearly describes the format, but it might be helpful to add a note that invalid formats will cause an error. This would make the CLI more user-friendly.
- help = "Specify which fields should have indices for specific models. Format: \"model_name:field1,field2;another_model:field3,field4\"" + help = "Specify which fields should have indices for specific models. Format: \"model_name:field1,field2;another_model:field3,field4\". Invalid formats will cause an error."
404-406: Consider providing more specific error messages, senseiWhile the current error is clear, it would be more helpful to provide details about what specifically was wrong with the format.
- return Err(anyhow::anyhow!("Invalid model indices format")); + return Err(anyhow::anyhow!( + "Invalid model indices format. Expected 'model_name:field1,field2', got '{}'", + part + ));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/cli/src/options.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (4)
crates/torii/cli/src/options.rs (4)
9-9: Ohayo! Clean import added for ModelIndicesNice addition of the ModelIndices import from torii_sqlite::types - this properly supports the new SQL indexing features being added.
375-398: Excellent implementation of granular SQL indexing options, sensei!The SqlOptions struct nicely addresses the concerns raised in the PR discussion about sensible defaults for database indexing. Setting
all_model_indicesto false by default aligns with the recommendation to only index keys by default, which is more performant than indexing every column.The model_indices parameter provides the flexibility to specify exactly which fields should have indices for specific models, enabling the granular control that this PR aims to introduce.
382-384: The help message formatting follows best practicesOhayo! I notice you've properly formatted the help message with line breaks to avoid exceeding line length limits. This addresses the previous CI pipeline failure mentioned in the past review comments.
400-412: Well-structured parsing function with proper error handlingOhayo! This parsing function cleanly handles the expected format for model indices. It correctly splits the input by ':' and ',' delimiters and validates the format before creating the ModelIndices struct.
The error messaging is clear when invalid formats are provided, which is important for CLI usability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/runner/src/lib.rs (1)
115-115: The cache size pragma is a great performance optimization.Ohayo! Nice addition of the SQLite cache size pragma, sensei! Setting it to 64MB will significantly improve query performance by keeping more data in memory, especially when dealing with many model indices.
You might want to consider making this cache size configurable through CLI arguments in the future, so users with different memory constraints can adjust it accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/runner/src/lib.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/torii/runner/src/lib.rs (2)
41-41: Import for SqlConfig looks good!Ohayo, sensei! I see you've added the SqlConfig import to handle the new granular model indices feature. This is a clean addition that properly separates configuration concerns.
152-161:❓ Verification inconclusive
Good implementation of granular model indices configuration.
Ohayo, sensei! I see you've successfully implemented the granular model indices feature as discussed in the PR comments. This addresses loothero's concern about not creating indices for every column by default.
The implementation allows users to:
- Enable all model indices (when needed) through
all_model_indices- Specify specific model indices through the
model_indicesparameter, with a safe fallback to default if not providedThis approach provides the flexibility needed while maintaining good performance by default.
🏁 Script executed:
#!/bin/bash # Let's check if there are tests for the new SqlConfig features rg -p "test.*SqlConfig" --type rust # Also check for usage examples in documentation rg -p "SqlConfig.*example" --type rustLength of output: 78
Ohayo, sensei! Granular model indices configuration looks solid—but please verify tests and docs manually.
The implementation in
crates/torii/runner/src/lib.rs(lines 152–161) correctly enables both the all-encompassing and granular configuration viaall_model_indicesandmodel_indices. However, our automated search didn’t turn up tests or usage examples specifically referencingSqlConfig. Could you manually confirm if tests or documentation examples are provided elsewhere or if they’re planned for a future update?
Summary by CodeRabbit
ModelIndicesstructure for defining model-specific fields.SqlConfigstructure to enhance SQL configuration management.