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

fix(torii-grpc): sql query for typed enums in nested arrays #2579

Merged
merged 7 commits into from
Oct 26, 2024
Merged

fix(torii-grpc): sql query for typed enums in nested arrays #2579

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3606647
fix(torii-grpc): sql query for typed enums in nested arrays
Larkooo Oct 24, 2024
2694fbe
fmt
Larkooo Oct 24, 2024
bc0bced
clippy
Larkooo Oct 25, 2024
f6b1423
fix: tests
Larkooo Oct 25, 2024
84bac2d
fmt
Larkooo Oct 25, 2024
592fbe1
fix: sql query
Larkooo Oct 25, 2024
a3ba920
update query
Larkooo Oct 25, 2024
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
116 changes: 83 additions & 33 deletions crates/torii/core/src/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,21 +234,37 @@ pub fn build_sql_query(
limit: Option<u32>,
offset: Option<u32>,
) -> Result<(String, HashMap<String, String>, String), Error> {
#[derive(Default)]
struct TableInfo {
table_name: String,
parent_table: Option<String>,
is_optional: bool,
depth: usize, // Track nesting depth for proper ordering
}
Comment on lines +237 to +243
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider Deriving Additional Traits for TableInfo

Ohayo sensei, to enhance usability and debugging, consider deriving Clone and Debug traits for the TableInfo struct.

Apply this diff to derive additional traits:

 #[derive(Default)]
+#[derive(Default, Clone, Debug)]
 struct TableInfo {
     table_name: String,
     parent_table: Option<String>,
     is_optional: bool,
     depth: usize, // Track nesting depth for proper ordering
 }

Committable suggestion was skipped due to low confidence.


#[allow(clippy::too_many_arguments)]
fn parse_ty(
path: &str,
name: &str,
ty: &Ty,
selections: &mut Vec<String>,
tables: &mut Vec<String>,
arrays_queries: &mut HashMap<String, (Vec<String>, Vec<String>)>,
tables: &mut Vec<TableInfo>,
arrays_queries: &mut HashMap<String, (Vec<String>, Vec<TableInfo>)>,
parent_is_optional: bool,
depth: usize,
Comment on lines +251 to +254
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the previous search results and the function definition, I can now provide the final response:

Ohayo sensei! Updates needed for parse_ty function calls

The parse_ty function calls in the codebase are missing the required parameters. The function signature requires 8 parameters, but all current calls in model.rs only pass 5 parameters or fewer:

  • path: &str
  • name: &str
  • ty: &Ty
  • selections: &mut Vec<String> (missing in calls)
  • tables: &mut Vec<TableInfo> (missing in calls)
  • arrays_queries: &mut HashMap<String, (Vec<String>, Vec<TableInfo>)> (missing in calls)
  • parent_is_optional: bool (missing in calls)
  • depth: usize (missing in calls)
  • Line 250-253: All calls to parse_ty in crates/torii/core/src/model.rs need to be updated with the missing parameters.
🔗 Analysis chain

Verify Updates to parse_ty Function Calls

Ohayo sensei, the parse_ty function now includes additional parameters parent_is_optional and depth. Please ensure all calls to parse_ty have been updated accordingly throughout the codebase to prevent potential runtime errors.

Run the following script to check for all invocations of parse_ty:

Let me gather more context about the parse_ty function definition to understand its parameters better.

Let me try a different approach to get the function definition.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all calls to parse_ty have the updated parameters.

# Search for calls to parse_ty and display surrounding lines.
rg 'parse_ty\(' -A3 -B1

Length of output: 3503

Script:

#!/bin/bash
# Get the full definition of parse_ty function
ast-grep --pattern 'fn parse_ty($$$)' -l rust

Length of output: 47

Script:

#!/bin/bash
# Get more context around the parse_ty function definition
rg -B5 -A15 'fn parse_ty\(' crates/torii/core/src/model.rs

Length of output: 712

) {
match &ty {
Ty::Struct(s) => {
// struct can be the main entrypoint to our model schema
// so we dont format the table name if the path is empty
let table_name =
if path.is_empty() { name.to_string() } else { format!("{}${}", path, name) };

tables.push(TableInfo {
table_name: table_name.clone(),
parent_table: if path.is_empty() { None } else { Some(path.to_string()) },
is_optional: parent_is_optional,
depth,
});

for child in &s.children {
parse_ty(
&table_name,
Expand All @@ -257,13 +273,21 @@ pub fn build_sql_query(
selections,
tables,
arrays_queries,
parent_is_optional,
depth + 1,
);
}

tables.push(table_name);
}
Ty::Tuple(t) => {
let table_name = format!("{}${}", path, name);

tables.push(TableInfo {
table_name: table_name.clone(),
parent_table: Some(path.to_string()),
is_optional: parent_is_optional,
depth,
});

for (i, child) in t.iter().enumerate() {
parse_ty(
&table_name,
Expand All @@ -272,16 +296,22 @@ pub fn build_sql_query(
selections,
tables,
arrays_queries,
parent_is_optional,
depth + 1,
);
}

tables.push(table_name);
}
Ty::Array(t) => {
let table_name = format!("{}${}", path, name);
let is_optional = true;

let mut array_selections = Vec::new();
let mut array_tables = vec![table_name.clone()];
let mut array_tables = vec![TableInfo {
table_name: table_name.clone(),
parent_table: Some(path.to_string()),
is_optional: true,
depth,
}];

parse_ty(
&table_name,
Expand All @@ -290,12 +320,15 @@ pub fn build_sql_query(
&mut array_selections,
&mut array_tables,
arrays_queries,
is_optional,
depth + 1,
);

arrays_queries.insert(table_name, (array_selections, array_tables));
}
Ty::Enum(e) => {
let table_name = format!("{}${}", path, name);
let is_optional = true;

let mut is_typed = false;
for option in &e.options {
Expand All @@ -312,26 +345,31 @@ pub fn build_sql_query(
selections,
tables,
arrays_queries,
is_optional,
depth + 1,
);
is_typed = true;
}

selections.push(format!("[{path}].external_{name} AS \"{path}.{name}\""));
selections.push(format!("[{}].external_{} AS \"{}.{}\"", path, name, path, name));
if is_typed {
tables.push(table_name);
tables.push(TableInfo {
table_name,
parent_table: Some(path.to_string()),
is_optional: parent_is_optional || is_optional,
depth,
});
}
}
_ => {
// alias selected columns to avoid conflicts in `JOIN`
selections.push(format!("[{path}].external_{name} AS \"{path}.{name}\""));
selections.push(format!("[{}].external_{} AS \"{}.{}\"", path, name, path, name));
}
}
}

let mut global_selections = Vec::new();
let mut global_tables = Vec::new();

let mut arrays_queries: HashMap<String, (Vec<String>, Vec<String>)> = HashMap::new();
let mut arrays_queries: HashMap<String, (Vec<String>, Vec<TableInfo>)> = HashMap::new();

for model in schemas {
parse_ty(
Expand All @@ -341,45 +379,56 @@ pub fn build_sql_query(
&mut global_selections,
&mut global_tables,
&mut arrays_queries,
false,
0,
);
}

// TODO: Fallback to subqueries, SQLite has a max limit of 64 on 'table 'JOIN'
if global_tables.len() > 64 {
return Err(QueryError::SqliteJoinLimit.into());
}

// Sort tables by depth to ensure proper join order
global_tables.sort_by_key(|table| table.depth);
Comment on lines +391 to +392
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider caching sorted tables for repeated queries.

Ohayo sensei! While sorting tables by depth ensures correct join order, consider caching the sorted tables if the same schema is used for multiple queries.

Consider adding a cached version of the sorted tables:

+    // Cache for sorted tables to avoid repeated sorting
+    static ref SORTED_TABLES_CACHE: Mutex<HashMap<String, Vec<TableInfo>>> = Mutex::new(HashMap::new());

     // Sort tables by depth to ensure proper join order
-    global_tables.sort_by_key(|table| table.depth);
+    let cache_key = format!("{:?}", schemas);
+    let sorted_tables = SORTED_TABLES_CACHE
+        .lock()
+        .unwrap()
+        .entry(cache_key)
+        .or_insert_with(|| {
+            let mut tables = global_tables.clone();
+            tables.sort_by_key(|table| table.depth);
+            tables
+        });

Committable suggestion was skipped due to low confidence.


let selections_clause = global_selections.join(", ");
let join_clause = global_tables
.into_iter()
.iter()
.map(|table| {
format!(" JOIN [{table}] ON {entities_table}.id = [{table}].{entity_relation_column}")
let join_type = if table.is_optional { "LEFT JOIN" } else { "JOIN" };
let join_condition =
format!("{entities_table}.id = [{}].{entity_relation_column}", table.table_name);
format!(" {join_type} [{}] ON {join_condition}", table.table_name)
})
.collect::<Vec<_>>()
.join(" ");

let mut formatted_arrays_queries: HashMap<String, String> = arrays_queries
.into_iter()
.map(|(table, (selections, tables))| {
.map(|(table, (selections, mut tables))| {
let mut selections_clause = selections.join(", ");
if !selections_clause.is_empty() {
selections_clause = format!(", {}", selections_clause);
}

// Sort array tables by depth
tables.sort_by_key(|table| table.depth);

let join_clause = tables
.iter()
.enumerate()
.map(|(idx, table)| {
if idx == 0 {
.map(|table| {
if table.parent_table.is_none() {
format!(
" JOIN [{table}] ON {entities_table}.id = \
[{table}].{entity_relation_column}"
" JOIN [{}] ON {entities_table}.id = [{}].{entity_relation_column}",
table.table_name, table.table_name
)
} else {
let join_type = if table.is_optional { "LEFT JOIN" } else { "JOIN" };
format!(
" JOIN [{table}] ON [{table}].full_array_id = \
[{prev_table}].full_array_id",
prev_table = tables[idx - 1]
" {join_type} [{}] ON [{}].full_array_id = [{}].full_array_id",
table.table_name,
table.table_name,
table.parent_table.as_ref().unwrap()
)
}
})
Expand All @@ -401,7 +450,7 @@ pub fn build_sql_query(
{entities_table}{join_clause}"
);
let mut count_query =
format!("SELECT COUNT({entities_table}.id) FROM {entities_table}{join_clause}",);
format!("SELECT COUNT({entities_table}.id) FROM {entities_table}{join_clause}");

if let Some(where_clause) = where_clause {
query += &format!(" WHERE {}", where_clause);
Expand Down Expand Up @@ -1023,11 +1072,12 @@ mod tests {
[Test-Position$vec].external_y AS \"Test-Position$vec.y\", \
[Test-PlayerConfig$favorite_item].external_Some AS \
\"Test-PlayerConfig$favorite_item.Some\", [Test-PlayerConfig].external_favorite_item \
AS \"Test-PlayerConfig.favorite_item\" FROM entities JOIN [Test-Position$vec] ON \
entities.id = [Test-Position$vec].entity_id JOIN [Test-Position] ON entities.id = \
[Test-Position].entity_id JOIN [Test-PlayerConfig$favorite_item] ON entities.id = \
[Test-PlayerConfig$favorite_item].entity_id JOIN [Test-PlayerConfig] ON entities.id \
= [Test-PlayerConfig].entity_id ORDER BY entities.event_id DESC";
AS \"Test-PlayerConfig.favorite_item\" FROM entities JOIN [Test-Position] ON \
entities.id = [Test-Position].entity_id JOIN [Test-PlayerConfig] ON entities.id = \
[Test-PlayerConfig].entity_id JOIN [Test-Position$vec] ON \
[Test-Position$vec].full_array_id = [Test-Position].full_array_id LEFT JOIN \
[Test-PlayerConfig$favorite_item] ON [Test-PlayerConfig$favorite_item].full_array_id \
= [Test-PlayerConfig].full_array_id ORDER BY entities.event_id DESC";
// todo: completely tests arrays
assert_eq!(query.0, expected_query);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/torii/grpc/src/types/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl From<Enum> for proto::types::Enum {
fn from(r#enum: Enum) -> Self {
proto::types::Enum {
name: r#enum.name,
option: r#enum.option.expect("option value") as u32,
option: r#enum.option.unwrap_or_default() as u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

which change makes it acceptable to have the default value in this case?

options: r#enum.options.into_iter().map(Into::into).collect::<Vec<_>>(),
}
}
Expand Down
Loading