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 14 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 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
b15d79f
increase cache size to 64mb
Larkooo Mar 3, 2025
241656f
default to only keys for indices
Larkooo Mar 3, 2025
792eef6
fmt
Larkooo Mar 3, 2025
6804a8f
clippy
Larkooo Mar 3, 2025
8831d16
fix
Larkooo Mar 3, 2025
7fedcf3
fmt
Larkooo Mar 3, 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
41 changes: 40 additions & 1 deletion 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 @@
#[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 Expand Up @@ -107,6 +110,10 @@
self.erc = config.erc.unwrap_or_default();
}

if self.sql == SqlOptions::default() {
self.sql = config.sql.unwrap_or_default();
}

#[cfg(feature = "server")]
{
if self.server == ServerOptions::default() {
Expand Down Expand Up @@ -136,6 +143,7 @@
pub indexing: Option<IndexingOptions>,
pub events: Option<EventsOptions>,
pub erc: Option<ErcOptions>,
pub sql: Option<SqlOptions>,
#[cfg(feature = "server")]
pub metrics: Option<MetricsOptions>,
#[cfg(feature = "server")]
Expand Down Expand Up @@ -167,6 +175,7 @@
config.events =
if args.events == EventsOptions::default() { None } else { Some(args.events) };
config.erc = if args.erc == ErcOptions::default() { None } else { Some(args.erc) };
config.sql = if args.sql == SqlOptions::default() { None } else { Some(args.sql) };

Check warning on line 178 in crates/torii/cli/src/args.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/cli/src/args.rs#L178 

Added line #L178 was not covered by tests

#[cfg(feature = "server")]
{
Expand All @@ -187,7 +196,7 @@
use std::net::{IpAddr, Ipv4Addr};
use std::str::FromStr;

use torii_sqlite::types::{Contract, ContractType};
use torii_sqlite::types::{Contract, ContractType, ModelIndices};

use super::*;

Expand All @@ -208,6 +217,10 @@
"ns-E",
"ns-EH"
]

[[sql.model_indices]]
model_tag = "ns-Position"
fields = ["vec.x", "vec.y"]
"#;
let path = std::env::temp_dir().join("torii-config2.json");
std::fs::write(&path, content).unwrap();
Expand All @@ -225,6 +238,8 @@
"--events.historical",
"a-A",
"--indexing.transactions",
"--sql.model_indices",
"ns-Position:vec.x,vec.y;ns-Moves:player",
"--config",
path_str.as_str(),
];
Expand All @@ -238,6 +253,19 @@
assert_eq!(torii_args.events.historical, vec!["a-A".to_string()]);
assert_eq!(torii_args.server, ServerOptions::default());
assert!(torii_args.indexing.transactions);
Comment on lines 253 to 255
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 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.

assert_eq!(
torii_args.sql.model_indices,
Some(vec![
ModelIndices {
model_tag: "ns-Position".to_string(),
fields: vec!["vec.x".to_string(), "vec.y".to_string()],
},
ModelIndices {
model_tag: "ns-Moves".to_string(),
fields: vec!["player".to_string()],
},
])
);
}

#[test]
Expand Down Expand Up @@ -269,6 +297,10 @@
"erc721:0x5678"
]
namespaces = []

[[sql.model_indices]]
model_tag = "ns-Position"
fields = ["vec.x", "vec.y"]
"#;
let path = std::env::temp_dir().join("torii-config.json");
std::fs::write(&path, content).unwrap();
Expand Down Expand Up @@ -303,6 +335,13 @@
}
]
);
assert_eq!(
torii_args.sql.model_indices,
Some(vec![ModelIndices {
model_tag: "ns-Position".to_string(),
fields: vec!["vec.x".to_string(), "vec.y".to_string()],
}])
);
assert_eq!(torii_args.server.http_addr, IpAddr::V4(Ipv4Addr::LOCALHOST));
assert_eq!(torii_args.server.http_port, 7777);
assert_eq!(torii_args.server.http_cors_origins, Some(vec!["*".to_string()]));
Expand Down
41 changes: 40 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 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,45 @@
}
}

#[derive(Debug, clap::Args, Clone, Serialize, Deserialize, PartialEq, Default)]
#[command(next_help_heading = "SQL options")]
pub struct SqlOptions {
/// Whether model tables should default to having indices on all columns
#[arg(
long = "sql.all_model_indices",
default_value_t = false,
help = "If true, creates indices on all columns of model tables by default. If false, \
only key fields columns of model tables will have indices."
)]
#[serde(default)]
pub all_model_indices: bool,

Check warning on line 386 in crates/torii/cli/src/options.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/cli/src/options.rs#L386 

Added line #L386 was not covered by tests

/// 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>>,
}

// 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"));

Check warning on line 405 in crates/torii/cli/src/options.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/cli/src/options.rs#L405 

Added line #L405 was not covered by tests
}

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
9 changes: 7 additions & 2 deletions 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::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 @@ -112,6 +112,7 @@
options = options.auto_vacuum(SqliteAutoVacuum::None);
options = options.journal_mode(SqliteJournalMode::Wal);
options = options.synchronous(SqliteSynchronous::Normal);
options = options.pragma("cache_size", "-65536"); // Set cache size to 64MB (65536 KiB)

Check warning on line 115 in crates/torii/runner/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/runner/src/lib.rs#L115 

Added line #L115 was not covered by tests

let pool = SqlitePoolOptions::new()
.min_connections(1)
Expand Down Expand Up @@ -148,11 +149,15 @@
let executor_handle = tokio::spawn(async move { executor.run().await });

let model_cache = Arc::new(ModelCache::new(readonly_pool.clone()));
let db = Sql::new(
let db = Sql::new_with_config(

Check warning on line 152 in crates/torii/runner/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/runner/src/lib.rs#L152 

Added line #L152 was not covered by tests
pool.clone(),
sender.clone(),
&self.args.indexing.contracts,
model_cache.clone(),
SqlConfig {
all_model_indices: self.args.sql.all_model_indices,
model_indices: self.args.sql.model_indices.unwrap_or_default(),
},

Check warning on line 160 in crates/torii/runner/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/runner/src/lib.rs#L157-L160 

Added lines #L157 - L160 were not covered by tests
)
.await?;

Expand Down
Loading
Loading