refactor: member clauses handle struct fields & operators & recursive #2420
Conversation
WalkthroughOhayo, sensei! The changes involve significant modifications to the Changes
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
Larkooo
force-pushed
the
composite-clause-powerful
branch
from
28baa66
to
133fd3a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
Line range hint
578-766: Ohayo sensei! The changes look great, just a minor suggestion.The refactoring to use
build_composite_clausemethod for constructing the SQL query is a significant improvement in terms of code readability and maintainability. The consistent usage offetch_oneand the improved approach to binding values are also positive changes.To further improve the code, consider extracting the logic for mapping the database entities to
proto::types::Entityinto a separate method. This will make thequery_by_compositemethod more focused and easier to understand.For example:
fn map_db_entities_to_proto_entities( db_entities: &Vec<(String, String)>, table: &str, entity_relation_column: &str, pool: &Pool<Sqlite>, ) -> Result<Vec<proto::types::Entity>, Error> { // ... }Then, in
query_by_composite:let entities = map_db_entities_to_proto_entities( &db_entities, table, entity_relation_column, &self.pool, ).await?;
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2420 +/- ##
==========================================
- Coverage 68.04% 68.01% -0.04%
==========================================
Files 364 364
Lines 47871 47892 +21
==========================================
- Hits 32573 32572 -1
- Misses 15298 15320 +22 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
527-528: Ohayo sensei! Consider simplifying the code.The code can be simplified by using
fetch_oneinstead offetch_optionaland handling theRowNotFounderror. Something like this:let models_str = sqlx::query_scalar(&models_query) .fetch_one(&self.pool) .await .map_err(|e| match e { sqlx::Error::RowNotFound => QueryError::NotFound, _ => e.into(), })?;
955-1051: Ohayo sensei! The newbuild_composite_clausefunction looks great.The function is well-structured and handles all clause types correctly. Recursively processing nested composite clauses is a nice touch. Using bind variables is also a good security practice.
Just one small nitpick - consider renaming
is_ortois_disjunctionoris_disjunctiveto be more explicit.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- crates/torii/grpc/src/server/mod.rs (7 hunks)
Additional comments not posted (3)
crates/torii/grpc/src/server/mod.rs (3)
44-44: LGTM!The import statement is correct and necessary.
543-550: Looks good to me, sensei!The parsing of the member clause correctly handles both top-level and nested fields. The use of
$as a separator andexternal_prefix for the column name follows the established conventions.
Line range hint
586-596: Ohayo! The changes look great.Extracting the clause building logic into
build_composite_clauseimproves code organization and readability. UsingDISTINCTin the count query is also a good catch to avoid duplicate counts.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
Line range hint
586-669: Ohayo sensei! The changes to use thebuild_composite_clausefunction look great!The
build_composite_clausefunction nicely encapsulates the logic for generating the SQL clauses and bind values for the composite clause query. This makes thequery_by_compositefunction much cleaner and easier to read.One minor nitpick: Consider using
if letinstead ofmatchfor themodels_strvariable since you only care about theSomecase:- let models_str: Option<String> = - sqlx::query_scalar(&models_query).fetch_optional(&self.pool).await?; - if models_str.is_none() { + if let Some(models_str) = sqlx::query_scalar(&models_query).fetch_optional(&self.pool).await? { + let model_ids = models_str + .split(',') + .map(Felt::from_str) + .collect::<Result<Vec<_>, _>>() + .map_err(ParseError::FromStr)?; + let schemas = + self.model_cache.models(&model_ids).await?.into_iter().map(|m| m.schema).collect(); + ... + } else { return Ok((Vec::new(), 0)); } - - let models_str = models_str.unwrap();
955-1051: Ohayo sensei! The newbuild_composite_clausefunction looks great!The function correctly generates the SQL clauses and bind values for composite clauses, handling all the different clause types and recursively handling nested composite clauses.
One minor nitpick: Consider using
String::new()instead ofString::from("")for empty strings. For example:- let having_clause = if !having_clauses.is_empty() { - format!("HAVING {}", having_clauses.join(if is_or { " OR " } else { " AND " })) - } else { - String::from("") + String::new() };This is a bit more idiomatic and avoids an unnecessary allocation.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
Line range hint
586-669: Ohayo sensei! The changes toquery_by_compositelook great!The refactor to use the new
build_composite_clausefunction improves the modularity and readability of the code by extracting the composite clause building logic into a separate function. 👍To further improve readability, consider extracting the count query and main query into separate functions. This will make the
query_by_compositefunction more focused and easier to understand.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- crates/torii/grpc/src/server/mod.rs (7 hunks)
Additional comments not posted (1)
crates/torii/grpc/src/server/mod.rs (1)
955-1050: Ohayo sensei! The newbuild_composite_clausefunction looks fantastic!The function correctly handles the different types of clauses (hashed keys, keys, member, and nested composite) and generates the appropriate SQL clauses and bind values. The recursive handling of nested composite clauses is a great feature that allows for complex queries.
The use of the
build_keys_patternfunction to generate the keys pattern for keys clauses, theComparisonOperatorenum to map the comparison operator for member clauses, and thePrimitivetype to convert the member clause value to a SQL value are all excellent choices that make the code more robust and maintainable.Overall, this is a well-designed and implemented function that greatly improves the composite clause handling in the codebase. Great job! 🙌
this PR reworks member clauses and composite clauses
Summary by CodeRabbit
New Features
Bug Fixes
Refactor