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
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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.44% +0.04%
==========================================
Files 440 440
Lines 59963 60046 +83
==========================================
+ Hits 34417 34491 +74
- Misses 25546 25555 +9 ☔ 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. |
Summary by CodeRabbit
ModelIndicesstructure for defining model-specific fields.SqlConfigstructure to enhance SQL configuration management.