feat(torii-grpc): IN operator for comparison #2812
Conversation
|
Warning Rate limit exceeded@Larkooo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughOhayo, sensei! The pull request introduces significant updates to the Changes
Possibly Related PRs
Suggested Reviewers
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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/proto/types.proto(1 hunks)
🔇 Additional comments (1)
crates/torii/grpc/proto/types.proto (1)
140-142: LGTM on the addition of 'MemberValueList' for 'in' comparisons
Ohayo, sensei! The addition of MemberValueList to support 'in' comparisons enhances the expressiveness of queries. Nicely done!
crates/torii/grpc/proto/types.proto
Outdated
| oneof comparison { | ||
| MemberValue eq = 4; | ||
| MemberValue neq = 5; | ||
| MemberValue gt = 6; | ||
| MemberValue gte = 7; | ||
| MemberValue lt = 8; | ||
| MemberValue lte = 9; | ||
| MemberValueList in = 10; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Ohayo! Update needed for MemberClause implementation in Rust code
The proto definition has moved from using operator and value fields to a oneof comparison field, but the Rust implementation in crates/torii/grpc/src/types/mod.rs still uses the old structure. Please update the Rust implementation to match the proto definition:
crates/torii/grpc/src/types/mod.rs: Replaceoperatorandvaluefields with acomparisonenum field that matches the proto definition.
🔗 Analysis chain
Ensure backward compatibility and update documentation for 'MemberClause'
Ohayo, sensei! The restructuring of MemberClause introduces a oneof comparison with multiple comparison types. Please verify that this change maintains backward compatibility with existing clients. Also, consider updating any related documentation to reflect the new structure.
Run the following script to check for usages of the old operator and value fields:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of the old 'operator' and 'value' fields in 'MemberClause'
# Expected: No remaining usages of 'operator' or 'value' in 'MemberClause' across the codebase
# Search for 'operator' field in 'MemberClause'
rg 'MemberClause.*operator'
# Search for 'value' field in 'MemberClause'
rg 'MemberClause.*value'
Length of output: 56
Script:
#!/bin/bash
# Let's try a more comprehensive search to understand the changes and their impact
# Check the git history of the proto file to see when these changes were made
git log -p -- crates/torii/grpc/proto/types.proto | head -n 50
# Search for any MemberClause usage patterns in the codebase
rg "MemberClause" -A 3 -B 3
# Look for any documentation files that might need updates
fd "README|CHANGELOG|CONTRIBUTING" --type f --exec cat {}
Length of output: 42574
Script:
#!/bin/bash
# Search for any test files or examples using MemberClause
fd "test|example" --type f --exec rg "MemberClause" {} \;
Length of output: 60
Script:
#!/bin/bash
# Check for any client implementations
rg -g "*.{rs,go,js,py,java}" "MemberClause"
Length of output: 422
Script:
#!/bin/bash
# Let's check the git history of the proto file to see the changes
git log -p crates/torii/grpc/proto/types.proto | grep -A 20 "message MemberClause"
# Check for any tests or implementations that might be affected
rg -g "*.rs" "impl.*MemberClause|test.*MemberClause" -B 3 -A 3
Length of output: 465
Script:
#!/bin/bash
# Look for any client code that might be consuming this proto
fd -e rs -e ts -e js -e py -e go -x rg "MemberClause.*comparison|MemberClause.*operator" {} \;
Length of output: 99
Script:
#!/bin/bash
# Let's check the current implementation of MemberClause in the codebase
rg -g "*.rs" "struct MemberClause" -B 5 -A 10
# And look for any proto message definitions that might be using it
rg -g "*.proto" "message MemberClause" -B 5 -A 15
Length of output: 2274
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/torii/grpc/src/types/mod.rs (1)
165-172: Consider wrapping string values in quotes infmt::DisplayforMemberValueWhen displaying
MemberValue::String, wrapping the string in quotes can improve clarity and distinguish it from other types.Apply this diff to wrap string values in quotes:
- MemberValue::String(string) => write!(f, "{}", string), + MemberValue::String(string) => write!(f, "\"{}\"", string),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/grpc/src/server/mod.rs(1 hunks)crates/torii/grpc/src/types/mod.rs(3 hunks)
🔇 Additional comments (4)
crates/torii/grpc/src/server/mod.rs (1)
1294-1294: Ohayo, sensei! The code update aligns with the new MemberClause structure
The change correctly accesses member.comparison.value, reflecting the updated structure. Looks good!
crates/torii/grpc/src/types/mod.rs (3)
196-207: Ohayo, sensei! The Comparison enum addition enhances expressiveness
The introduction of the Comparison enum with various comparison variants improves the flexibility of member comparisons. Nice work!
209-239: All comparison variants are correctly handled in fmt::Display
The fmt::Display implementation for Comparison handles all variants appropriately, ensuring proper formatting. Well done!
434-444: Ohayo, sensei! Correctly updated TryFrom for MemberClause
The TryFrom implementation for MemberClause now properly handles the new comparison field, ensuring accurate conversion.
crates/torii/grpc/src/types/mod.rs
Outdated
| impl TryFrom<proto::types::MemberValue> for MemberValue { | ||
| type Error = SchemaError; | ||
| fn try_from(value: proto::types::MemberValue) -> Result<Self, Self::Error> { | ||
| Ok(match value.value_type { | ||
| Some(proto::types::member_value::ValueType::Primitive(primitive)) => MemberValue::Primitive(primitive.try_into()?), | ||
| Some(proto::types::member_value::ValueType::String(string)) => MemberValue::String(string), | ||
| None => return Err(SchemaError::MissingExpectedData("value_type".into())), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error handling in TryFrom for MemberValue
In the implementation of TryFrom<proto::types::MemberValue> for MemberValue, errors from primitive.try_into() might not match the SchemaError type specified. Consider mapping or converting the error types to ensure consistency.
crates/torii/grpc/src/types/mod.rs
Outdated
| impl TryFrom<proto::types::member_clause::Comparison> for Comparison { | ||
| type Error = FromStrError; | ||
| fn try_from(operator: proto::types::member_clause::Comparison) -> Result<Self, Self::Error> { | ||
| Ok(match operator { | ||
| proto::types::member_clause::Comparison::Eq(value) => Comparison::Eq(value.try_into()?), | ||
| proto::types::member_clause::Comparison::Gte(value) => Comparison::Gte(value.try_into()?), | ||
| proto::types::member_clause::Comparison::Gt(value) => Comparison::Gt(value.try_into()?), | ||
| proto::types::member_clause::Comparison::Lt(value) => Comparison::Lt(value.try_into()?), | ||
| proto::types::member_clause::Comparison::Lte(value) => Comparison::Lte(value.try_into()?), | ||
| proto::types::member_clause::Comparison::Neq(value) => Comparison::Neq(value.try_into()?), | ||
| proto::types::member_clause::Comparison::In(values) => { | ||
| Comparison::In(values.values.into_iter().map(|v| v.try_into()).collect::<Result<Vec<_>, _>>()?) | ||
| } | ||
| proto::types::member_clause::Comparison::NotIn(values) => { | ||
| Comparison::NotIn(values.values.into_iter().map(|v| v.try_into()).collect::<Result<Vec<_>, _>>()?) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review error type in TryFrom implementation for Comparison
The impl TryFrom<proto::types::member_clause::Comparison> for Comparison specifies an Error type of FromStrError, but the try_into() calls can produce SchemaError. Consider aligning the error types for consistency.
Larkooo
force-pushed
the
filter-in-grpc
branch
from
8da21e7 to
2c18b72
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
845-845: Inconsistent error handling patterns detected, sensei!The value extraction pattern differs slightly between
query_by_memberandbuild_composite_clause:// In query_by_member member_clause.value.clone().ok_or(QueryError::MissingParam("value".into()))? // In build_composite_clause member.value.clone().ok_or(QueryError::MissingParam("value".into()))?While both work correctly, maintaining consistent error handling patterns across the codebase improves maintainability.
Consider extracting this common pattern into a helper function to ensure consistent error handling:
fn extract_member_value(value: Option<proto::types::MemberValue>) -> Result<proto::types::MemberValue, Error> { value.ok_or_else(|| QueryError::MissingParam("value".into()).into()) }Also applies to: 1364-1364
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/grpc/proto/types.proto(2 hunks)crates/torii/grpc/src/server/mod.rs(3 hunks)crates/torii/grpc/src/types/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/grpc/src/types/mod.rs
- crates/torii/grpc/proto/types.proto
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
776-788: Ohayo! Nice implementation of the comparison value extraction logic!
The function correctly handles all value types including the new list support for IN operator comparisons.
crates/torii/grpc/src/server/mod.rs
Outdated
| fn comparison_value(value: &proto::types::MemberValue) -> Result<String, Error> { | ||
| match &value.value_type { | ||
| Some(ValueType::String(value)) => Ok(value.to_string()), | ||
| Some(ValueType::Primitive(value)) => { | ||
| let primitive: Primitive = value.try_into()?; | ||
| primitive.to_sql_value() | ||
| let primitive: Primitive = (value.clone()).try_into()?; | ||
| Ok(primitive.to_sql_value()) | ||
| } | ||
| Some(ValueType::List(values)) => { | ||
| Ok(format!("({})", values.values.iter().map(|v| comparison_value(v)).collect::<Result<Vec<String>, Error>>()?.join(", "))) | ||
| } | ||
| None => return Err(QueryError::MissingParam("value_type".into()).into()), | ||
| }; | ||
| bind_values.push(comparison_value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Code duplication detected, sensei!
The comparison_value function is duplicated. Consider extracting it to a single location, perhaps as a private function at module level, to maintain DRY principles.
Apply this refactor to remove the duplication:
// Add at module level
-fn comparison_value(value: &proto::types::MemberValue) -> Result<String, Error> {
- match &value.value_type {
- Some(ValueType::String(value)) => Ok(value.to_string()),
- Some(ValueType::Primitive(value)) => {
- let primitive: Primitive = (value.clone()).try_into()?;
- Ok(primitive.to_sql_value())
- }
- Some(ValueType::List(values)) => {
- Ok(format!("({})", values.values.iter().map(|v| comparison_value(v)).collect::<Result<Vec<String>, Error>>()?.join(", ")))
- }
- None => return Err(QueryError::MissingParam("value_type".into()).into()),
- }
-}
// Remove the first implementation and use the module level function
// Remove the second implementation and use the module level functionCommittable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2812 +/- ##
==========================================
- Coverage 55.67% 55.62% -0.05%
==========================================
Files 439 439
Lines 55699 55744 +45
==========================================
- Hits 31009 31008 -1
- Misses 24690 24736 +46 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
INandNOT_INcomparison types.MemberValueListmessage for handling lists of member values.