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.

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-grpc): start rework to use 1 single query #2817

Merged
merged 23 commits into from
Dec 21, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
27 changes: 25 additions & 2 deletions crates/torii/core/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,10 @@ impl ModelReader<Error> for ModelSQLReader {
pub fn build_sql_query(
schemas: &Vec<Ty>,
table_name: &str,
model_relation_table: &str,
entity_relation_column: &str,
where_clause: Option<&str>,
having_clause: Option<&str>,
order_by: Option<&str>,
limit: Option<u32>,
offset: Option<u32>,
Expand Down Expand Up @@ -172,6 +174,7 @@ pub fn build_sql_query(
// Add base table columns
selections.push(format!("{}.id", table_name));
selections.push(format!("{}.keys", table_name));
selections.push(format!("group_concat({model_relation_table}.model_id) as model_ids"));

// Process each model schema
for model in schemas {
Expand All @@ -185,19 +188,37 @@ pub fn build_sql_query(
collect_columns(&model_table, "", model, &mut selections);
}

joins.push(format!(
"JOIN {model_relation_table} ON {table_name}.id = {model_relation_table}.entity_id"
));

Comment on lines +191 to +194
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider using LEFT JOIN for model relations.

Using JOIN might exclude entities without model relations. Consider using LEFT JOIN to ensure all entities are included in the result set.

-    joins.push(format!(
-        "JOIN {model_relation_table} ON {table_name}.id = {model_relation_table}.entity_id"
-    ));
+    joins.push(format!(
+        "LEFT JOIN {model_relation_table} ON {table_name}.id = {model_relation_table}.entity_id"
+    ));
📝 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
joins.push(format!(
"JOIN {model_relation_table} ON {table_name}.id = {model_relation_table}.entity_id"
));
joins.push(format!(
"LEFT JOIN {model_relation_table} ON {table_name}.id = {model_relation_table}.entity_id"
));

let selections_clause = selections.join(", ");
let joins_clause = joins.join(" ");

let mut query = format!("SELECT {} FROM [{}] {}", selections_clause, table_name, joins_clause);

let mut count_query =
format!("SELECT COUNT(DISTINCT {}.id) FROM [{}] {}", table_name, table_name, joins_clause);
// Include model_ids in the subquery and put WHERE before GROUP BY
let mut count_query = format!(
"SELECT COUNT(*) FROM (SELECT {}.id, group_concat({}.model_id) as model_ids FROM [{}] {}",
table_name, model_relation_table, table_name, joins_clause
);

if let Some(where_clause) = where_clause {
query += &format!(" WHERE {}", where_clause);
count_query += &format!(" WHERE {}", where_clause);
}

query += &format!(" GROUP BY {table_name}.id");
count_query += &format!(" GROUP BY {table_name}.id");

if let Some(having_clause) = having_clause {
query += &format!(" HAVING {}", having_clause);
count_query += &format!(" HAVING {}", having_clause);
}

// Close the subquery
count_query += ") AS filtered_entities";

// Use custom order by if provided, otherwise default to event_id DESC
if let Some(order_clause) = order_by {
query += &format!(" ORDER BY {}", order_clause);
Expand Down Expand Up @@ -490,11 +511,13 @@ mod tests {
let query = build_sql_query(
&vec![position, player_config],
"entities",
"entity_model",
"internal_entity_id",
None,
None,
None,
None,
None,
Comment on lines +514 to +520
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo! Enhance test coverage for new query features.

The test case has been updated with the new parameters, but it doesn't verify the behavior of GROUP BY, HAVING clause, or model_ids aggregation.

Add test cases to verify:

  1. GROUP BY behavior with aggregated model_ids
  2. HAVING clause filtering
  3. Count query results with different combinations of parameters
#[test]
fn test_query_with_aggregation() {
    // Test with model_ids aggregation
}

#[test]
fn test_query_with_having() {
    // Test HAVING clause filtering
}

)
.unwrap();

Expand Down
4 changes: 4 additions & 0 deletions crates/torii/core/src/sql/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ impl ModelCache {
}

pub async fn models(&self, selectors: &[Felt]) -> Result<Vec<Model>, Error> {
if selectors.is_empty() {
return Ok(self.model_cache.read().await.values().cloned().collect());
}

let mut schemas = Vec::with_capacity(selectors.len());
for selector in selectors {
schemas.push(self.model(selector).await?);
Expand Down
Loading