feat: introduce updated at field in top level torii query - #2807
Conversation
WalkthroughOhayo, sensei! This pull request introduces modifications across several files to enhance the handling of an Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/core/src/model.rs (1)
Line range hint
176-189: Ohayo, sensei! Potential inefficiency ininternal_updated_atclause construction.The
internal_updated_at_clausevector is initialized even ifinternal_updated_atis zero. Consider initializing it only wheninternal_updated_at > 0to optimize performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/core/src/model.rs(5 hunks)crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/src/server/mod.rs(17 hunks)crates/torii/grpc/src/types/mod.rs(2 hunks)
🔇 Additional comments (12)
crates/torii/grpc/src/server/mod.rs (7)
Line range hint 232-244: Ohayo, sensei! Verify consistent propagation of internal_updated_at parameter.
The entities_all function now includes the internal_updated_at parameter. Please ensure that this parameter is consistently passed to all internal function calls within entities_all, such as query_by_hashed_keys, to maintain proper filtering based on the update timestamp.
271-271: Ohayo, sensei! Confirm usage of internal_updated_at in fetch_entities.
The fetch_entities function now includes the internal_updated_at parameter. Please verify that this parameter is utilized within the function, particularly when constructing queries, to ensure entities are filtered correctly.
Line range hint 438-519: Ohayo, sensei! Ensure internal_updated_at is passed to fetch_entities correctly.
In the query_by_hashed_keys function, the new internal_updated_at parameter is added. Please verify that this parameter is correctly passed to the fetch_entities function to maintain consistency in entity filtering.
Line range hint 537-666: Ohayo, sensei! Check internal update timestamp handling in query_by_keys.
The query_by_keys function now includes internal_updated_at. Ensure that this parameter affects the query as intended and that it is passed correctly to any subsequent functions or query constructions.
Line range hint 778-812: Ohayo, sensei! Validate internal_updated_at implementation in query_by_member.
Please ensure that the internal_updated_at parameter is correctly utilized within the query_by_member function, especially when building and executing queries.
Line range hint 883-1052: Ohayo, sensei! Review internal_updated_at handling in query_by_composite.
In the query_by_composite function, the internal_updated_at parameter is introduced. Please confirm that it is correctly integrated into the query logic and that it properly filters entities based on the update timestamp.
Line range hint 1052-1121: Ohayo, sensei! Ensure internal_updated_at is passed and utilized in retrieve_entities.
The retrieve_entities function now accepts the internal_updated_at parameter. Verify that this parameter is consistently passed to the appropriate functions and that it effectively influences the entity retrieval based on the update timestamp.
crates/torii/grpc/proto/types.proto (1)
79-79: Ohayo, sensei! Confirm field numbering for internal_updated_at.
The new field internal_updated_at is added with field number 7. Ensure that this field number does not collide with any existing or reserved field numbers, and that the addition maintains backward compatibility.
crates/torii/grpc/src/types/mod.rs (2)
109-109: Ohayo, sensei! Adding internal_updated_at to Query struct looks good.
The inclusion of internal_updated_at in the Query struct aligns with the protocol buffer definition and ensures consistency between client and server.
275-275: Ohayo, sensei! Verify internal_updated_at field mapping in From<Query>.
Ensure that the internal_updated_at field is correctly mapped in the From<Query> implementation to prevent any serialization issues.
crates/torii/core/src/model.rs (2)
127-127: Ohayo, sensei! Addition of internal_updated_at parameter to build_sql_query.
The new parameter allows filtering based on update timestamps, enhancing query capabilities.
207-218: Ohayo, sensei! Correct conditional inclusion of internal_updated_at in WHERE clause.
The logic to append the internal_updated_at conditions to the WHERE clause is correctly handled, ensuring proper SQL query construction.
| internal_updated_at, | ||
| )?; | ||
|
|
||
| let rows = sqlx::query(&entity_query).bind(models_str).fetch_all(&mut *tx).await?; | ||
| let query = sqlx::query(&entity_query).bind(models_str); | ||
| let rows = query.fetch_all(&mut *tx).await?; |
There was a problem hiding this comment.
Ohayo, sensei! Missing binding for internal_updated_at in the SQL query.
While internal_updated_at is used within the build_sql_query function to build the SQL query, it appears that in lines 363-364, internal_updated_at is not bound to the query. This may lead to incorrect query results.
Please bind internal_updated_at to the query to ensure proper functionality.
Apply this diff to fix the issue:
- let query = sqlx::query(&entity_query).bind(models_str);
+ let query = sqlx::query(&entity_query).bind(models_str).bind(internal_updated_at);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal_updated_at, | |
| )?; | |
| let rows = sqlx::query(&entity_query).bind(models_str).fetch_all(&mut *tx).await?; | |
| let query = sqlx::query(&entity_query).bind(models_str); | |
| let rows = query.fetch_all(&mut *tx).await?; | |
| internal_updated_at, | |
| )?; | |
| let query = sqlx::query(&entity_query).bind(models_str).bind(internal_updated_at); | |
| let rows = query.fetch_all(&mut *tx).await?; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
crates/torii/grpc/src/server/mod.rs (1)
Line range hint
332-361: Ohayo, sensei! Consider optimizing model cache access.The code fetches models from cache in a loop, which could be optimized by fetching all required models at once and caching them locally.
Consider refactoring to batch the model cache access:
-let schemas = self - .model_cache - .models(&model_ids) - .await? - .into_iter() - .map(|m| m.schema) - .collect::<Vec<_>>(); +// Fetch all models at once at the beginning +let model_cache = self.model_cache.models(&all_model_ids).await?; +let model_map: HashMap<_, _> = model_cache.into_iter().map(|m| (m.id, m.schema)).collect();crates/torii/grpc/src/server/tests/entities_test.rs (1)
146-146: LGTM! Test updated for the new internal_updated_at parameter.The test now includes the new timestamp parameter with a value of 0, which effectively disables timestamp filtering. This provides good coverage for the base case.
Consider adding additional test cases with non-zero timestamps to verify the filtering behavior.
crates/torii/core/src/model.rs (2)
Line range hint
177-190: Consider optimizing the vector capacity initialization.The
internal_updated_at_clausevector's capacity is set toschemas.len(), but it will only be populated ifinternal_updated_at > 0. Consider moving the initialization inside the condition to avoid unnecessary allocations.- let mut internal_updated_at_clause = Vec::with_capacity(schemas.len()); // Process each model schema for model in schemas { let model_table = model.name(); joins.push(format!( "LEFT JOIN [{model_table}] ON {table_name}.id = \ [{model_table}].{entity_relation_column}", )); + let mut internal_updated_at_clause = if internal_updated_at > 0 { + Vec::with_capacity(schemas.len()) + } else { + Vec::new() + }; if internal_updated_at > 0 { internal_updated_at_clause .push(format!("[{model_table}].internal_updated_at > {}", internal_updated_at)); }
517-517: LGTM! Test updated with the new parameter.The test case has been updated to include the new
internal_updated_atparameter with a value of 0.Consider adding test cases with non-zero timestamps to verify the filtering behavior, similar to the suggestion for the integration test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/core/src/model.rs(6 hunks)crates/torii/grpc/src/server/mod.rs(18 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(1 hunks)
🔇 Additional comments (3)
crates/torii/grpc/src/server/mod.rs (1)
364-365:
Ohayo, sensei! Missing binding for internal_updated_at in the SQL query.
The SQL query is built with internal_updated_at parameter but it's not bound to the query execution. This could lead to incorrect filtering of results.
Apply this diff to fix the issue:
-let query = sqlx::query(&entity_query).bind(models_str);
+let query = sqlx::query(&entity_query).bind(models_str).bind(internal_updated_at);crates/torii/core/src/model.rs (2)
Line range hint 119-128: LGTM! Function signature updated appropriately.
Ohayo! The new internal_updated_at parameter is well-placed and the clippy attribute is maintained correctly.
208-219: LGTM! SQL query construction handles timestamp filtering correctly.
The implementation properly handles both cases:
- When there's no existing WHERE clause
- When there's an existing WHERE clause that needs to be extended
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2807 +/- ##
==========================================
- Coverage 55.74% 55.71% -0.04%
==========================================
Files 439 439
Lines 55613 55657 +44
==========================================
+ Hits 31001 31008 +7
- Misses 24612 24649 +37 ☔ View full report in Codecov by Sentry. |
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)
365-374: Ohayo, sensei! Consider optimizing timestamp binding.The timestamp is being bound multiple times (once per schema) which is redundant since it's the same value. Consider binding it only once and reusing the parameter in the SQL query.
- if internal_updated_at > 0 { - for _ in 0..schemas.len() { - query = query.bind( - DateTime::<Utc>::from_timestamp(internal_updated_at as i64, 0) - .ok_or_else(|| Error::from(QueryError::UnsupportedQuery))? - .to_rfc3339(), - ); - } - } + if internal_updated_at > 0 { + query = query.bind( + DateTime::<Utc>::from_timestamp(internal_updated_at as i64, 0) + .ok_or_else(|| Error::from(QueryError::UnsupportedQuery))? + .to_rfc3339(), + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/core/src/model.rs(6 hunks)crates/torii/grpc/src/server/mod.rs(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/model.rs
🔇 Additional comments (2)
crates/torii/grpc/src/server/mod.rs (2)
233-233: Ohayo, sensei! Parameter propagation looks good!
The internal_updated_at parameter is consistently propagated through all query methods, maintaining a clean and uniform interface.
Also applies to: 245-245, 273-273, 362-362, 449-449, 548-548, 722-722, 823-823, 1063-1063, 1087-1087, 1102-1102, 1117-1117, 1132-1132
362-362: Ohayo, sensei! Previous binding issue has been resolved.
The timestamp binding issue mentioned in the past review has been properly addressed. The internal_updated_at parameter is now correctly passed to build_sql_query and bound to the query.
Also applies to: 365-365
edisontim
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
Line range hint
233-245: Suggestion: Refactor to reduce the number of function parametersSeveral methods have a large number of parameters, leading to
clippy::too_many_argumentswarnings. Consider grouping related parameters into structs or creating builder patterns to improve code readability and maintainability.Also applies to: 264-274, 458-539, 557-689, 731-798, 832-906
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/core/src/error.rs(1 hunks)crates/torii/core/src/model.rs(6 hunks)crates/torii/grpc/src/server/mod.rs(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/model.rs
🔇 Additional comments (1)
crates/torii/core/src/error.rs (1)
54-55: Ohayo, sensei! The addition of InvalidTimestamp error variant looks good
The new InvalidTimestamp(u64) variant appropriately handles invalid timestamp errors, enhancing error reporting for invalid timestamps.
| let time = DateTime::<Utc>::from_timestamp(internal_updated_at as i64, 0) | ||
| .ok_or_else(|| { | ||
| Error::from(QueryError::InvalidTimestamp(internal_updated_at)) | ||
| })? | ||
| .to_rfc3339(); |
There was a problem hiding this comment.
Ohayo, sensei! Fix incorrect timestamp conversion and handle possible overflow
- The
DateTime::<Utc>::from_timestampmethod does not exist. UseUtc.timestamp_optinstead. - Casting
internal_updated_atfromu64toi64can cause overflow if the value exceedsi64::MAX. Ensureinternal_updated_atis within the valid range before casting.
Apply this diff to fix the code:
+ if internal_updated_at > i64::MAX as u64 {
+ return Err(Error::from(QueryError::InvalidTimestamp(internal_updated_at)));
+ }
- let time = DateTime::<Utc>::from_timestamp(internal_updated_at as i64, 0)
- .ok_or_else(|| {
- Error::from(QueryError::InvalidTimestamp(internal_updated_at))
- })?
- .to_rfc3339();
+ let time = Utc.timestamp_opt(internal_updated_at as i64, 0)
+ .single()
+ .ok_or_else(|| Error::from(QueryError::InvalidTimestamp(internal_updated_at)))?
+ .to_rfc3339();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let time = DateTime::<Utc>::from_timestamp(internal_updated_at as i64, 0) | |
| .ok_or_else(|| { | |
| Error::from(QueryError::InvalidTimestamp(internal_updated_at)) | |
| })? | |
| .to_rfc3339(); | |
| if internal_updated_at > i64::MAX as u64 { | |
| return Err(Error::from(QueryError::InvalidTimestamp(internal_updated_at))); | |
| } | |
| let time = Utc.timestamp_opt(internal_updated_at as i64, 0) | |
| .single() | |
| .ok_or_else(|| Error::from(QueryError::InvalidTimestamp(internal_updated_at)))? | |
| .to_rfc3339(); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
373-373: Ohayo, sensei! Optimize timestamp binding by avoiding unnecessary clones.The
time.clone()operation is unnecessary as the timestamp string can be reused without cloning.Apply this diff to optimize the code:
- query = query.bind(time.clone()); + query = query.bind(&time);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(19 hunks)
🔇 Additional comments (3)
crates/torii/grpc/src/server/mod.rs (3)
362-362: Ohayo, sensei! Missing binding for internal_updated_at in the SQL query.
The internal_updated_at parameter is passed to build_sql_query but not bound to the query, which may lead to incorrect query results.
368-372: Ohayo, sensei! Fix incorrect timestamp conversion and handle possible overflow.
The timestamp conversion code needs to be updated to use Utc.timestamp_opt and handle potential overflow when casting from u64 to i64.
233-233: Ohayo, sensei! Well-structured propagation of internal_updated_at.
The internal_updated_at parameter is consistently propagated through all query methods, maintaining a clean and organized codebase.
Also applies to: 245-245, 273-273, 362-362, 1065-1065, 1089-1089, 1104-1104, 1119-1119, 1134-1134
@glihm @Larkooo I've also found that I can do this way more efficiently (~33% reduction time in query) by querying the |
|
Let's maybe add an InternalClause or EntitiesClause that way it'll be confusing and still be used within the clause systems |
Let's sync on that, as yeah I think generally speaking a lot of feedback to take from such a big game, and where Torii can be improved. For the moment, will merge and move forward to unlock the dojo.js integration. |
glihm
left a comment
glihm
left a comment
There was a problem hiding this comment.
Thank @edisontim!
Let's keep iterating on that with @Larkooo and others to ensure we learn from those use cases for Torii evolution.
In the future, could remove the internal_ prefix. 👍
| if internal_updated_at > 0 { | ||
| internal_updated_at_clause.push(format!("[{model_table}].internal_updated_at >= ?")); |
There was a problem hiding this comment.
Here there's a potential speedup if we can trust the updated_at that is inside the entities table. Which could avoid having this WHERE clause added for each models.
3 participants
Summary by CodeRabbit
New Features
internal_updated_atfor tracking updates in theQuerymessage.internal_updated_attimestamp.entity_modelsto specify which models should be retrieved.Bug Fixes
InvalidTimestampvariant in theQueryErrorenum.Documentation
internal_updated_atparameter across various functions.