-
Notifications
You must be signed in to change notification settings - Fork 189
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
Changes from 2 commits
8a81615
060cf0e
3a45876
9a4d414
ae5fc97
754805e
55ed216
b0d3683
ee9f43a
496364f
78bc49e
343d1e0
99049e0
56e4e6d
39002ac
e05122f
ef3b12c
124bc8f
d5c4e74
745cf35
3a1136c
759c5b8
feecbdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -464,95 +464,77 @@ impl DojoWorld { | |
} | ||
}; | ||
|
||
// count query that matches filter_ids | ||
let count_query = format!( | ||
r#" | ||
SELECT count(*) | ||
FROM {table} | ||
{where_clause} | ||
"# | ||
); | ||
|
||
// total count of rows without limit and offset | ||
let mut count_query = sqlx::query_scalar(&count_query); | ||
if let Some(hashed_keys) = &hashed_keys { | ||
for key in &hashed_keys.hashed_keys { | ||
let key = Felt::from_bytes_be_slice(key); | ||
count_query = count_query.bind(format!("{:#x}", key)); | ||
if table == EVENT_MESSAGES_HISTORICAL_TABLE { | ||
let count_query = format!( | ||
r#" | ||
SELECT count(*) | ||
FROM {table} | ||
{where_clause} | ||
"# | ||
); | ||
let total_count = sqlx::query_scalar(&count_query) | ||
.bind(entity_updated_after.clone()) | ||
.fetch_one(&self.pool) | ||
.await?; | ||
if total_count == 0 { | ||
return Ok((Vec::new(), 0)); | ||
} | ||
} | ||
|
||
if let Some(entity_updated_after) = entity_updated_after.clone() { | ||
count_query = count_query.bind(entity_updated_after); | ||
} | ||
let total_count = count_query.fetch_optional(&self.pool).await?.unwrap_or(0); | ||
if total_count == 0 { | ||
return Ok((Vec::new(), 0)); | ||
let entities = | ||
self.fetch_historical_event_messages(&format!( | ||
r#" | ||
SELECT {table}.id, {table}.data, {table}.model_id, group_concat({model_relation_table}.model_id) as model_ids | ||
FROM {table} | ||
JOIN {model_relation_table} ON {table}.id = {model_relation_table}.entity_id | ||
{where_clause} | ||
GROUP BY {table}.event_id | ||
ORDER BY {table}.event_id DESC | ||
"# | ||
), None, limit, offset).await?; | ||
return Ok((entities, total_count)); | ||
} | ||
|
||
// Query to get entity IDs and their model IDs | ||
let mut query = if table == EVENT_MESSAGES_HISTORICAL_TABLE { | ||
format!( | ||
r#" | ||
SELECT {table}.id, {table}.data, {table}.model_id, group_concat({model_relation_table}.model_id) as model_ids | ||
FROM {table} | ||
JOIN {model_relation_table} ON {table}.id = {model_relation_table}.entity_id | ||
{where_clause} | ||
GROUP BY {table}.event_id | ||
ORDER BY {table}.event_id DESC | ||
"# | ||
) | ||
} else { | ||
format!( | ||
r#" | ||
SELECT {table}.id, group_concat({model_relation_table}.model_id) as model_ids | ||
let count_query = format!( | ||
r#" | ||
SELECT count(*) | ||
FROM {table} | ||
JOIN {model_relation_table} ON {table}.id = {model_relation_table}.entity_id | ||
{where_clause} | ||
GROUP BY {table}.id | ||
ORDER BY {table}.event_id DESC | ||
"# | ||
) | ||
}; | ||
|
||
if limit.is_some() { | ||
query += " LIMIT ?" | ||
} | ||
|
||
if offset.is_some() { | ||
query += " OFFSET ?" | ||
} | ||
|
||
if table == EVENT_MESSAGES_HISTORICAL_TABLE { | ||
let entities = | ||
self.fetch_historical_event_messages(&query, None, limit, offset).await?; | ||
return Ok((entities, total_count)); | ||
} | ||
"# | ||
); | ||
|
||
let mut query = sqlx::query_as(&query); | ||
// retrieve all schemas | ||
let schemas = self | ||
.model_cache | ||
.models(&[]) | ||
.await? | ||
.iter() | ||
.map(|m| m.schema.clone()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Performance concern when loading all models. In lines 505-511, calling Consider specifying only the required model selectors or implementing pagination to limit the number of models loaded. |
||
.collect::<Vec<_>>(); | ||
let (query, count_query) = build_sql_query( | ||
&schemas, | ||
table, | ||
entity_relation_column, | ||
Some(&where_clause), | ||
order_by, | ||
limit, | ||
offset, | ||
)?; | ||
let query = sqlx::query(&query); | ||
if let Some(hashed_keys) = hashed_keys { | ||
for key in hashed_keys.hashed_keys { | ||
let key = Felt::from_bytes_be_slice(&key); | ||
query = query.bind(format!("{:#x}", key)); | ||
} | ||
} | ||
|
||
if let Some(entity_updated_after) = entity_updated_after.clone() { | ||
query = query.bind(entity_updated_after); | ||
} | ||
query = query.bind(limit).bind(offset); | ||
let db_entities: Vec<(String, String)> = query.fetch_all(&self.pool).await?; | ||
let entities = query.fetch_all(&self.pool).await?; | ||
let entities = entities | ||
.iter() | ||
.map(|row| map_row_to_entity(row, &schemas, dont_include_hashed_keys)) | ||
.collect::<Result<Vec<_>, Error>>()?; | ||
|
||
let entities = self | ||
.fetch_entities( | ||
table, | ||
entity_relation_column, | ||
db_entities, | ||
dont_include_hashed_keys, | ||
order_by, | ||
entity_models, | ||
) | ||
.await?; | ||
Ok((entities, total_count)) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohayo, sensei! Conditional binding of
entity_updated_after
.In lines 476-477,
entity_updated_after.clone()
is bound to thecount_query
without checking if it'sSome
. Ifentity_updated_after
isNone
, this could lead to unintended SQL queries or errors. Ensure that you bindentity_updated_after
only when it's present.Apply this diff to conditionally bind
entity_updated_after
:This ensures that
entity_updated_after
is only bound when it's available.📝 Committable suggestion