fix(torii-grpc): sql query for typed enums in nested arrays #2579
Conversation
WalkthroughOhayo, sensei! The changes in this pull request involve significant enhancements to the SQL query generation process in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SQLGenerator
participant Database
Client->>SQLGenerator: Request SQL Query
SQLGenerator->>SQLGenerator: Build SQL Query with TableInfo
SQLGenerator->>Database: Execute SQL Query
Database-->>SQLGenerator: Return Query Results
SQLGenerator-->>Client: Send Query Results
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
crates/torii/core/src/model.rs (4)
257-265: Simplifytable_nameAssignmentOhayo sensei, the assignment of
table_namecan be simplified for better readability using string formatting.Apply this diff to simplify the code:
let table_name = - if path.is_empty() { name.to_string() } else { format!("{}${}", path, name) }; + let table_name = format!("{}${}", path, name).trim_start_matches('$').to_string();
463-464: Unused Variablecount_queryOhayo sensei, the variable
count_queryis defined but not used later in thebuild_sql_queryfunction. Consider utilizing it if needed or removing it to clean up the code.Apply this diff to remove the unused variable:
let mut count_query = format!("SELECT COUNT({entities_table}.id) FROM {entities_table}{join_clause}"); + +// Removed unused variable count_query as it is not used later in the function
Line range hint
505-510: Handle Potential Errors inmap_row_to_tyFunctionOhayo sensei, in the
map_row_to_tyfunction, when retrieving array rows, theexpect("qed; rows should exist")may cause a panic if the key is missing. Consider handling this case more gracefully to prevent unexpected crashes.Apply this diff to handle missing keys:
// Retrieve rows for the array -let rows = arrays_rows - .get(&path) - .expect("qed; rows should exist") +let rows = if let Some(rows) = arrays_rows.get(&path) { + rows +} else { + // Handle the case where rows do not exist + return Ok(()); +} .iter() .filter(|array_row| array_row.get::<String, _>("id") == row.get::<String, _>("id")) .collect::<Vec<_>>();
Line range hint
555-557: Add Missing Field Initializations in TestsOhayo sensei, in the test cases within
tests, some instances ofSqlModelMemberare missing theidfield initialization. Ensure all required fields are properly initialized to prevent future issues.For example, update the test data as follows:
SqlModelMember { id: "Test-Position".into(), + id: "some_id".into(), name: "x".into(), // ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- crates/torii/core/src/model.rs (7 hunks)
- crates/torii/grpc/src/types/schema.rs (1 hunks)
🔇 Additional comments (2)
crates/torii/grpc/src/types/schema.rs (1)
116-116: Ohayo sensei! Great improvement in Option handling!The change from
expect()tounwrap_or_default()is a robust improvement that eliminates potential runtime panics when theoptionfield isNone. This aligns perfectly with Rust's philosophy of graceful error handling.Let's verify if there are any other similar patterns in the codebase that could benefit from this improvement:
✅ Verification successful
The first script will help us understand the context of the
expectusage inmodel.rs, and the second will check if there are any other similar patterns inschema.rsthat might need attention. This will give us a more complete picture to make a final recommendation.
Ohayo sensei! Your Option handling improvement is spot on!
The change from
expect()tounwrap_or_default()is indeed a great improvement. After verification, I found only one similar case inmodel.rsbut it's actually a different scenario - it usesexpectwith "qed" comment indicating it's an invariant that should never fail, which is an acceptable use case.The current change in
schema.rsis perfect as is, making the code more robust by gracefully handling theNonecase instead of potentially panicking.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potentially dangerous expect() calls in enum handling rg 'expect.*enum' --type rustLength of output: 236
Script:
#!/bin/bash # Let's check the context around the expect in torii/core/src/model.rs rg -B 5 -A 5 'expect.*enum_options' crates/torii/core/src/model.rs # Also check for any other expect patterns in schema.rs rg 'expect' crates/torii/grpc/src/types/schema.rsLength of output: 641
crates/torii/core/src/model.rs (1)
355-360: Ensure Correct Handling of Typed EnumsOhayo sensei, when handling typed enums, verify that the
table_nameis correctly added totablesonly when necessary. This ensures accurate SQL query generation.
crates/torii/core/src/model.rs
Outdated
| 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) |
There was a problem hiding this comment.
Review Join Conditions in SQL Query
Ohayo sensei, there might be a logical issue in the join conditions within the join_clause. The condition for tables with no parent seems to reference entities_table.id joined to [table_name].{entity_relation_column}, but the logic may need to be adjusted based on the actual schema.
Please double-check the join conditions to ensure they correctly reflect the relationships between tables.
| #[derive(Default)] | ||
| struct TableInfo { | ||
| table_name: String, | ||
| parent_table: Option<String>, | ||
| is_optional: bool, | ||
| depth: usize, // Track nesting depth for proper ordering | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| tables: &mut Vec<TableInfo>, | ||
| arrays_queries: &mut HashMap<String, (Vec<String>, Vec<TableInfo>)>, | ||
| parent_is_optional: bool, | ||
| depth: usize, |
There was a problem hiding this comment.
💡 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: &strname: &strty: &Tyselections: &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_tyincrates/torii/core/src/model.rsneed 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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/core/src/model.rs (1)
237-243: Add documentation for TableInfo struct.Ohayo sensei! The
TableInfostruct is well-structured, but would benefit from documentation explaining its purpose and fields.Add rustdoc comments:
#[derive(Default)] +/// Represents metadata about a database table in the query. struct TableInfo { + /// The name of the table table_name: String, + /// The parent table name, if this table is nested parent_table: Option<String>, + /// Whether this table's relationship is optional is_optional: bool, + /// Nesting depth used for proper join ordering depth: usize, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/torii/core/src/model.rs (7 hunks)
🔇 Additional comments (2)
crates/torii/core/src/model.rs (2)
Line range hint
244-276: LGTM: Improved handling of nested relationships.The changes to
parse_tyenhance the handling of table relationships by:
- Tracking nesting depth for proper join ordering
- Propagating optionality information through the hierarchy
- Maintaining parent-child relationships
397-406: Verify join conditions for complex nested structures.The join conditions look correct, but let's verify the handling of deeply nested structures.
✅ Verification successful
Join conditions are correctly handling nested structures
Ohayo sensei! After reviewing the implementation, I can confirm that the join conditions are properly handling nested structures. The code shows:
- Correct depth tracking for nested structures via the
depthfield- Proper parent-child relationships maintained through
parent_table- Appropriate join type selection (
LEFT JOINfor optional relationships)- Consistent
full_array_idbased joining for nested arrays- Proper table name generation with
$delimiter for nested structuresThe join condition logic correctly handles both root-level joins (
entities_table.id) and nested joins (full_array_id), ensuring proper relationship maintenance throughout the hierarchy.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues with deeply nested joins # Look for nested array patterns that might cause issues ast-grep --pattern 'Ty::Array(vec![Ty::Array($$$)])' # Check for complex join conditions rg -A 2 'JOIN.*JOIN.*JOIN.*JOIN'Length of output: 89
Script:
#!/bin/bash # Let's try a different approach to verify the join conditions # Search for array-related type definitions and nested structures ast-grep --pattern 'struct $_ { $$$ array: $_, $$$ }' # Look for join-related test cases rg -l "test.*join" # Search for SQL generation logic involving joins rg -A 5 "JOIN.*table_name" # Check for array-related field handling ast-grep --pattern 'fn $_($$_) -> $_ { $$$ full_array_id $$$ }'Length of output: 774
Script:
#!/bin/bash # Let's check the test cases and implementation details more specifically # Look for test cases involving nested arrays or complex joins rg -A 10 "test.*nested.*array|test.*complex.*join" crates/torii/ # Check the model.rs file for array-related table handling rg -A 5 "parent_table|is_optional|table_name" crates/torii/core/src/model.rs # Search for SQL query generation with full_array_id rg -A 3 "full_array_id.*JOIN|JOIN.*full_array_id" crates/torii/core/src/ # Look for array relationship handling in the schema ast-grep --pattern 'impl $_ { $$$ fn $_($$_) -> $_ { $$$ parent_table $$$ } }'Length of output: 5428
| // Sort tables by depth to ensure proper join order | ||
| global_tables.sort_by_key(|table| table.depth); |
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/core/src/model.rs (1)
1083-1088: Add test cases for edge cases.Ohayo sensei! Consider adding test cases for:
- Maximum nesting depth scenarios
- Complex enum types in deeply nested arrays
- Multiple optional nested structures
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/torii/core/src/model.rs (8 hunks)
🔇 Additional comments (3)
crates/torii/core/src/model.rs (3)
237-243: Add documentation for the TableInfo struct.Ohayo sensei! Consider adding documentation to explain the purpose and usage of each field in the TableInfo struct, especially the depth field's role in join ordering.
The previous suggestion about deriving Clone and Debug traits is still valid.
391-439: LGTM! Well-structured join generation logic.Ohayo sensei! The implementation correctly:
- Sorts tables by depth to ensure proper join order
- Uses appropriate join types based on optionality
- Constructs correct join conditions for nested structures
Line range hint
245-277: Add depth limit validation.Ohayo sensei! Consider adding a depth limit check to prevent potential stack overflow from deeply nested structures. This is especially important since the depth parameter is used for join ordering.
fn parse_ty( path: &str, name: &str, ty: &Ty, selections: &mut Vec<String>, tables: &mut Vec<TableInfo>, arrays_queries: &mut HashMap<String, (Vec<String>, Vec<TableInfo>)>, parent_is_optional: bool, depth: usize, ) { + const MAX_DEPTH: usize = 32; // Reasonable limit for nested structures + if depth >= MAX_DEPTH { + panic!("Maximum nesting depth exceeded"); + } match &ty {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/torii/core/src/model.rs (3)
237-243: Add documentation and derive Debug for TableInfo.Ohayo sensei! The TableInfo struct is well-structured, but would benefit from documentation explaining its purpose and fields. Also, deriving Debug would help with debugging and error messages.
Apply this diff:
#[derive(Default)] +#[derive(Default, Debug)] +/// Represents metadata about a database table including its relationships and nesting depth struct TableInfo { + /// Name of the database table table_name: String, + /// Parent table name if this table is nested within another parent_table: Option<String>, + /// Whether this table's relationship is optional is_optional: bool, + /// Nesting depth used for proper join ordering depth: usize, }
391-392: Consider caching sorted tables for repeated queries.Ohayo sensei! While sorting tables by depth ensures correct join order, we could optimize performance by caching the sorted tables when the same schema is used multiple times.
Consider implementing a caching mechanism using a concurrent HashMap to store sorted tables keyed by schema hash. This would be particularly beneficial for frequently accessed schemas.
1075-1080: Add test cases for edge scenarios.Ohayo sensei! While the current test coverage is good, consider adding test cases for:
- Deeply nested optional relationships (>3 levels)
- Mixed optional and required relationships in the same branch
- Edge cases around the SQLite join limit
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/torii/core/src/model.rs (8 hunks)
🔇 Additional comments (1)
crates/torii/core/src/model.rs (1)
354-361: 🛠️ Refactor suggestionOptimize enum table creation logic.
Ohayo sensei! The enum handling could be optimized by moving the table creation outside the
is_typedcheck since we're already pushing the selection regardless.Consider this optimization:
- selections.push(format!("[{}].external_{} AS \"{}.{}\"", path, name, path, name)); - if is_typed { - tables.push(TableInfo { - table_name, - parent_table: Some(path.to_string()), - is_optional: parent_is_optional || is_optional, - depth, - }); - } + tables.push(TableInfo { + table_name: table_name.clone(), + parent_table: Some(path.to_string()), + is_optional: parent_is_optional || is_optional, + depth, + }); + selections.push(format!("[{}].external_{} AS \"{}.{}\"", path, name, path, name));Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/torii/core/src/model.rs (3)
237-243: Consider deriving additional traits for TableInfo.Ohayo sensei! The TableInfo struct looks good, but could benefit from deriving
DebugandClonetraits for better debugging and manipulation capabilities.-#[derive(Default)] +#[derive(Default, Debug, Clone)] struct TableInfo { table_name: String, parent_table: Option<String>, is_optional: bool, depth: usize, }
391-392: 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 to avoid repeated sorting operations.
1075-1080: Consider adding explicit test cases for join ordering.Ohayo sensei! While the current test verifies the final query string, it would be beneficial to add specific test cases that verify the correct ordering of joins based on table depth.
Consider adding a test case like this:
#[test] fn test_join_order() { // Create a schema with tables at different depths let schema = create_nested_schema(); let (query, _, _) = build_sql_query(&vec![schema], "entities", "entity_id", None, None, None, None).unwrap(); // Verify that parent tables are joined before child tables assert!(query.find("JOIN [parent]").unwrap() < query.find("JOIN [child]").unwrap()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/torii/core/src/model.rs (8 hunks)
🔇 Additional comments (1)
crates/torii/core/src/model.rs (1)
354-361: 🛠️ Refactor suggestionConsider optimizing enum type handling.
Ohayo sensei! The current implementation pushes the table info after checking
is_typed, but we can optimize by moving the selection push after the table info push to maintain better logical grouping.- selections.push(format!("[{}].external_{} AS \"{}.{}\"", path, name, path, name)); - if is_typed { - tables.push(TableInfo { - table_name, - parent_table: Some(path.to_string()), - is_optional: parent_is_optional || is_optional, - depth, - }); - } + if is_typed { + tables.push(TableInfo { + table_name, + parent_table: Some(path.to_string()), + is_optional: parent_is_optional || is_optional, + depth, + }); + } + selections.push(format!("[{}].external_{} AS \"{}.{}\"", path, name, path, name));Likely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2579 +/- ##
==========================================
+ Coverage 69.59% 69.61% +0.02%
==========================================
Files 401 401
Lines 50760 50810 +50
==========================================
+ Hits 35327 35373 +46
- Misses 15433 15437 +4 ☔ View full report in Codecov by Sentry. |
| @@ -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, | |||
There was a problem hiding this comment.
which change makes it acceptable to have the default value in this case?
Summary by CodeRabbit
TableInfostruct for enhanced SQL query construction.Fromimplementation for theEnumstruct to prevent runtime panics.