Skip to content
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.

Sign up for GitHub

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

feat(torii-sql): granular model indices #3073

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+355 −218
Open

feat(torii-sql): granular model indices #3073

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
098ce9a
feat(torii-sql): granular model indices
Larkooo Feb 28, 2025
fb0488f
sql config
Larkooo Feb 28, 2025
f60aeef
new iwth config
Larkooo Feb 28, 2025
795330b
fmt
Larkooo Feb 28, 2025
4051f80
allow too many arguments
Larkooo Feb 28, 2025
80e4ea6
add config file support
Larkooo Feb 28, 2025
6c636f8
add tests
Larkooo Feb 28, 2025
2a01d08
fmt
Larkooo Feb 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/torii/cli/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ pub struct ToriiArgs {
#[command(flatten)]
pub erc: ErcOptions,

#[command(flatten)]
pub sql: SqlOptions,

Comment on lines +54 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.rs

Length 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 ToriiArgsConfig to include an equivalent Option<SqlOptions> field.
  • Also, revise the configuration file loading methods (like with_config_file and the TryFrom<ToriiArgs> implementation) to properly process the new SQL options.

#[cfg(feature = "server")]
#[command(flatten)]
pub metrics: MetricsOptions,
Expand Down
46 changes: 45 additions & 1 deletion crates/torii/cli/src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use camino::Utf8PathBuf;
use serde::ser::SerializeSeq;
use serde::{Deserialize, Serialize};
use starknet::core::types::Felt;
use torii_sqlite::types::{Contract, ContractType};
use torii_sqlite::types::{Contract, ContractType, ModelIndices};

pub const DEFAULT_HTTP_ADDR: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST);
pub const DEFAULT_HTTP_PORT: u16 = 8080;
Expand Down Expand Up @@ -372,6 +372,50 @@ impl Default for ErcOptions {
}
}

#[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>>,
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
#[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.


impl Default for SqlOptions {
fn default() -> Self {
Self { model_indices_keys: false, model_indices: None }
}
}

// Parses clap cli argument which is expected to be in the format:
// - model-tag:field1,field2;othermodel-tag:field3,field4
fn parse_model_indices(part: &str) -> anyhow::Result<ModelIndices> {
let parts = part.split(':').collect::<Vec<&str>>();
if parts.len() != 2 {
return Err(anyhow::anyhow!("Invalid model indices format"));
}

let model_tag = parts[0].to_string();
let fields = parts[1].split(',').map(|s| s.to_string()).collect::<Vec<_>>();

Ok(ModelIndices { model_tag, fields })
}

// Parses clap cli argument which is expected to be in the format:
// - erc_type:address:start_block
// - address:start_block (erc_type defaults to ERC20)
Expand Down
5 changes: 4 additions & 1 deletion crates/torii/runner/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use torii_sqlite::cache::ModelCache;
use torii_sqlite::executor::Executor;
use torii_sqlite::simple_broker::SimpleBroker;
use torii_sqlite::types::{Contract, ContractType, Model};
use torii_sqlite::Sql;
use torii_sqlite::{Sql, SqlConfig};
use tracing::{error, info};
use tracing_subscriber::{fmt, EnvFilter};
use url::form_urlencoded;
Expand Down Expand Up @@ -153,6 +153,9 @@ impl Runner {
sender.clone(),
&self.args.indexing.contracts,
model_cache.clone(),
SqlConfig {
model_indices: self.args.sql.model_indices.unwrap_or_default(),
},
)
.await?;

Expand Down
Loading
Loading