From 36066472284e78ef80ae85f016035c004d8f22b9 Mon Sep 17 00:00:00 2001 From: Nasr Date: Thu, 24 Oct 2024 13:01:42 -0400 Subject: [PATCH 1/7] fix(torii-grpc): sql query for typed enums in nested arrays --- crates/torii/core/src/model.rs | 111 ++++++++++++++++++++------ crates/torii/grpc/src/types/schema.rs | 2 +- 2 files changed, 86 insertions(+), 27 deletions(-) diff --git a/crates/torii/core/src/model.rs b/crates/torii/core/src/model.rs index b753685142..d46c356569 100644 --- a/crates/torii/core/src/model.rs +++ b/crates/torii/core/src/model.rs @@ -234,21 +234,36 @@ pub fn build_sql_query( limit: Option, offset: Option, ) -> Result<(String, HashMap, String), Error> { + #[derive(Default)] + struct TableInfo { + table_name: String, + parent_table: Option, + is_optional: bool, + depth: usize, // Track nesting depth for proper ordering + } + fn parse_ty( path: &str, name: &str, ty: &Ty, selections: &mut Vec, - tables: &mut Vec, - arrays_queries: &mut HashMap, Vec)>, + tables: &mut Vec, + arrays_queries: &mut HashMap, Vec)>, + parent_is_optional: bool, + depth: usize, ) { 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 = + 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, @@ -257,13 +272,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, @@ -272,16 +295,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, @@ -290,12 +319,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 { @@ -312,26 +344,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, Vec)> = HashMap::new(); + let mut arrays_queries: HashMap, Vec)> = HashMap::new(); for model in schemas { parse_ty( @@ -341,45 +378,67 @@ 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); + 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 = if table.parent_table.is_none() { + format!("{entities_table}.id = [{}].{entity_relation_column}", + table.table_name) + } else { + format!("[{}].full_array_id = [{}].full_array_id", + table.table_name, + table.parent_table.as_ref().unwrap()) + }; + format!(" {join_type} [{}] ON {join_condition}", + table.table_name) }) .collect::>() .join(" "); let mut formatted_arrays_queries: HashMap = 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 { 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() ) } }) @@ -401,7 +460,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); diff --git a/crates/torii/grpc/src/types/schema.rs b/crates/torii/grpc/src/types/schema.rs index e31f9c2271..058b014965 100644 --- a/crates/torii/grpc/src/types/schema.rs +++ b/crates/torii/grpc/src/types/schema.rs @@ -113,7 +113,7 @@ impl From 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, options: r#enum.options.into_iter().map(Into::into).collect::>(), } } From 2694fbe1cff1d120defafa3862d90db06b776cbf Mon Sep 17 00:00:00 2001 From: Nasr Date: Thu, 24 Oct 2024 13:12:47 -0400 Subject: [PATCH 2/7] fmt --- crates/torii/core/src/model.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/crates/torii/core/src/model.rs b/crates/torii/core/src/model.rs index d46c356569..d559350c6d 100644 --- a/crates/torii/core/src/model.rs +++ b/crates/torii/core/src/model.rs @@ -254,7 +254,7 @@ pub fn build_sql_query( ) { match &ty { Ty::Struct(s) => { - let table_name = + let table_name = if path.is_empty() { name.to_string() } else { format!("{}${}", path, name) }; tables.push(TableInfo { @@ -396,15 +396,15 @@ pub fn build_sql_query( .map(|table| { let join_type = if table.is_optional { "LEFT JOIN" } else { "JOIN" }; let join_condition = if table.parent_table.is_none() { - format!("{entities_table}.id = [{}].{entity_relation_column}", - table.table_name) + format!("{entities_table}.id = [{}].{entity_relation_column}", table.table_name) } else { - format!("[{}].full_array_id = [{}].full_array_id", + format!( + "[{}].full_array_id = [{}].full_array_id", table.table_name, - table.parent_table.as_ref().unwrap()) + table.parent_table.as_ref().unwrap() + ) }; - format!(" {join_type} [{}] ON {join_condition}", - table.table_name) + format!(" {join_type} [{}] ON {join_condition}", table.table_name) }) .collect::>() .join(" "); @@ -426,16 +426,13 @@ pub fn build_sql_query( .map(|(idx, table)| { if idx == 0 { format!( - " JOIN [{}] ON {entities_table}.id = \ - [{}].{entity_relation_column}", - table.table_name, - table.table_name + " 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_type} [{}] ON [{}].full_array_id = \ - [{}].full_array_id", + " {join_type} [{}] ON [{}].full_array_id = [{}].full_array_id", table.table_name, table.table_name, table.parent_table.as_ref().unwrap() From bc0bced176a712e40920dfbd191df9ec6a2b14b6 Mon Sep 17 00:00:00 2001 From: Nasr Date: Fri, 25 Oct 2024 10:09:05 -0400 Subject: [PATCH 3/7] clippy --- crates/torii/core/src/model.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/torii/core/src/model.rs b/crates/torii/core/src/model.rs index d559350c6d..bf820a87ab 100644 --- a/crates/torii/core/src/model.rs +++ b/crates/torii/core/src/model.rs @@ -242,6 +242,7 @@ pub fn build_sql_query( depth: usize, // Track nesting depth for proper ordering } + #[allow(clippy::too_many_arguments)] fn parse_ty( path: &str, name: &str, From f6b1423486f9a82c2a7dccbef3d629ed8e4febcb Mon Sep 17 00:00:00 2001 From: Nasr Date: Fri, 25 Oct 2024 10:51:13 -0400 Subject: [PATCH 4/7] fix: tests --- crates/torii/core/src/model.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/crates/torii/core/src/model.rs b/crates/torii/core/src/model.rs index bf820a87ab..6cb7c38841 100644 --- a/crates/torii/core/src/model.rs +++ b/crates/torii/core/src/model.rs @@ -1075,16 +1075,15 @@ mod tests { .unwrap(); let expected_query = - "SELECT entities.id, entities.keys, [Test-Position].external_player AS \ - \"Test-Position.player\", [Test-Position$vec].external_x AS \"Test-Position$vec.x\", \ - [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"; + "SELECT entities.id, entities.keys, [Test-Position].external_player AS \"Test-Position.player\", \ + [Test-Position$vec].external_x AS \"Test-Position$vec.x\", [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] 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); } From 84bac2d6dfa9b23e50a7b631906d4fdbe223bf61 Mon Sep 17 00:00:00 2001 From: Nasr Date: Fri, 25 Oct 2024 10:51:29 -0400 Subject: [PATCH 5/7] fmt --- crates/torii/core/src/model.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/crates/torii/core/src/model.rs b/crates/torii/core/src/model.rs index 6cb7c38841..e892abdcfe 100644 --- a/crates/torii/core/src/model.rs +++ b/crates/torii/core/src/model.rs @@ -1075,15 +1075,17 @@ mod tests { .unwrap(); let expected_query = - "SELECT entities.id, entities.keys, [Test-Position].external_player AS \"Test-Position.player\", \ - [Test-Position$vec].external_x AS \"Test-Position$vec.x\", [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] 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"; + "SELECT entities.id, entities.keys, [Test-Position].external_player AS \ + \"Test-Position.player\", [Test-Position$vec].external_x AS \"Test-Position$vec.x\", \ + [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] 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); } From 592fbe15099cb1789d3335b1ab90d54a492d7d55 Mon Sep 17 00:00:00 2001 From: Nasr Date: Fri, 25 Oct 2024 16:18:50 -0400 Subject: [PATCH 6/7] fix: sql query --- crates/torii/core/src/model.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/crates/torii/core/src/model.rs b/crates/torii/core/src/model.rs index e892abdcfe..af65909863 100644 --- a/crates/torii/core/src/model.rs +++ b/crates/torii/core/src/model.rs @@ -396,15 +396,8 @@ pub fn build_sql_query( .iter() .map(|table| { let join_type = if table.is_optional { "LEFT JOIN" } else { "JOIN" }; - let join_condition = if table.parent_table.is_none() { - format!("{entities_table}.id = [{}].{entity_relation_column}", table.table_name) - } else { - format!( - "[{}].full_array_id = [{}].full_array_id", - table.table_name, - table.parent_table.as_ref().unwrap() - ) - }; + let join_condition = + format!("{entities_table}.id = [{}].{entity_relation_column}", table.table_name); format!(" {join_type} [{}] ON {join_condition}", table.table_name) }) .collect::>() @@ -423,9 +416,8 @@ pub fn build_sql_query( let join_clause = tables .iter() - .enumerate() - .map(|(idx, table)| { - if idx == 0 { + .map(|table| { + if table.parent_table.is_none() { format!( " JOIN [{}] ON {entities_table}.id = [{}].{entity_relation_column}", table.table_name, table.table_name From a3ba920d70c8f2f6dc3545dd877cb3ec69607ca8 Mon Sep 17 00:00:00 2001 From: Nasr Date: Fri, 25 Oct 2024 16:50:14 -0400 Subject: [PATCH 7/7] update query --- crates/torii/core/src/model.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/torii/core/src/model.rs b/crates/torii/core/src/model.rs index af65909863..af31f86d76 100644 --- a/crates/torii/core/src/model.rs +++ b/crates/torii/core/src/model.rs @@ -1074,10 +1074,10 @@ mod tests { \"Test-PlayerConfig$favorite_item.Some\", [Test-PlayerConfig].external_favorite_item \ 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"; + [Test-PlayerConfig].entity_id JOIN [Test-Position$vec] ON entities.id = \ + [Test-Position$vec].entity_id LEFT JOIN [Test-PlayerConfig$favorite_item] ON \ + entities.id = [Test-PlayerConfig$favorite_item].entity_id ORDER BY entities.event_id \ + DESC"; // todo: completely tests arrays assert_eq!(query.0, expected_query); }