Feat-5410: add op and operand in filter #5442
Conversation
rocketmq-rust-bot
requested review from
TeslaRustor,
mxsm and
rocketmq-rust-bot
rocketmq-rust-robot
added
Difficulty level/Moderate
|
🔊@albertbolt1 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
WalkthroughRemoved Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rocketmq-rust-bot
added
AI review first
![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
🧹 Nitpick comments (2)
rocketmq-common/src/common/filter/operand.rs (1)
10-12: Rename parameter fromnametosymbolfor consistency.The parameter should be named
symbolrather thannameto align with the OpBase API and the domain concept.🔎 Proposed fix
- pub fn new(name: &str) -> Self { - Self { op: OpBase::new(name) } + pub fn new(symbol: &str) -> Self { + Self { op: OpBase::new(symbol) } }rocketmq-common/src/common/filter/op.rs (1)
18-30: Consider implementing Op trait for OpBase.
OpBasehas the same interface as theOptrait (asymbol()method) but doesn't implement the trait. This creates inconsistency and limits composability. IfOpBaseis meant to be used as an operator, it should implementOpdirectly.🔎 Proposed implementation
impl OpBase { pub fn new(symbol: impl Into<String>) -> Self { Self { symbol: symbol.into() } } pub fn symbol(&self) -> &str { &self.symbol } } + +impl Op for OpBase { + fn symbol(&self) -> &str { + &self.symbol + } +}This would allow
OpBaseto be used directly where anOpis expected, andOperandcould still delegate to it through the trait.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rocketmq-common/src/common/filter.rsrocketmq-common/src/common/filter/op.rsrocketmq-common/src/common/filter/operand.rs
🧰 Additional context used
🧬 Code graph analysis (2)
rocketmq-common/src/common/filter/op.rs (2)
rocketmq-common/src/common/filter/operand.rs (2)
symbol(16-18)new(10-12)rocketmq-common/src/utils/parse_config_file.rs (1)
cfg(30-30)
rocketmq-common/src/common/filter/operand.rs (2)
rocketmq-common/src/common/filter/op.rs (3)
new(24-26)symbol(15-15)symbol(27-29)rocketmq-common/src/utils/parse_config_file.rs (1)
cfg(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Code Coverage
- GitHub Check: Build & Test (ubuntu-latest)
- GitHub Check: Build & Test (windows-latest)
- GitHub Check: auto-approve
🔇 Additional comments (6)
rocketmq-common/src/common/filter.rs (1)
17-18: LGTM! Module declarations follow existing patterns.The new module declarations are consistent with the existing filter submodules and properly expose the Op and Operand types.
rocketmq-common/src/common/filter/operand.rs (3)
1-2: LGTM! Imports are correct.
4-19: Clarify the semantic relationship between Operand and Op.In typical expression parsing systems, operands (values/variables) and operators (+, -, *, etc.) are distinct concepts. Having
Operandimplement theOptrait seems semantically backwards.If this design is preserved from the Java implementation for compatibility, consider adding documentation to explain why operands are treated as operators in this context.
Is this design intentional from the Java port, or should operands and operators have separate trait hierarchies?
21-31: LGTM! Basic test coverage is adequate.The unit test correctly verifies the construction and symbol retrieval functionality.
rocketmq-common/src/common/filter/op.rs (2)
14-16: LGTM! Trait design is sound.The
Send + Syncbounds are appropriate for concurrent usage in RocketMQ, and the trait interface is clean and focused.
1-13: LGTM! License header is correct.
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5442 +/- ##
==========================================
+ Coverage 37.83% 38.05% +0.22%
==========================================
Files 811 813 +2
Lines 109903 110079 +176
==========================================
+ Hits 41585 41896 +311
+ Misses 68318 68183 -135 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rocketmq-common/src/common/filter/operand.rs (4)
14-15: Consider consolidating imports from the same module.The two imports from
crate::common::filter::opcan be combined into a singleusestatement for better readability.🔎 Proposed consolidation
-use crate::common::filter::op::Op; -use crate::common::filter::op::OpBase; +use crate::common::filter::op::{Op, OpBase};
22-28: Add documentation for the public constructor.Public API methods should include doc comments explaining their purpose, parameters, and usage examples.
🔎 Suggested documentation
impl Operand { + /// Creates a new `Operand` with the given symbol. + /// + /// # Arguments + /// + /// * `symbol` - The operator symbol (e.g., "+", "-", "*") + /// + /// # Example + /// + /// ``` + /// use rocketmq_common::common::filter::operand::Operand; + /// let operand = Operand::new("+"); + /// ``` pub fn new(symbol: &str) -> Self { Self { op: OpBase::new(symbol), } } }
36-46: Consider expanding test coverage.The basic test validates core functionality, which is good. However, consider adding test cases for edge cases and different symbol types to improve confidence in the implementation.
🔎 Suggested additional tests
#[test] fn create_operand_with_different_symbols() { let symbols = vec!["+", "-", "*", "/", "==", "!=", ">", "<"]; for sym in symbols { let operand = Operand::new(sym); assert_eq!(operand.symbol(), sym); } } #[test] fn create_operand_with_empty_string() { let operand = Operand::new(""); assert_eq!(operand.symbol(), ""); } #[test] fn operand_clone_works() { let operand = Operand::new("+"); let cloned = operand.clone(); assert_eq!(cloned.symbol(), "+"); }
17-20: Consider removing the unnecessaryOpBasewrapper inOperand.
OperandwrapsOpBase, which wraps aString. SinceOpBaseis only used byOperandand provides no additional behavior beyond storing and accessing a symbol string,Operandcan directly contain theStringfield. This would simplify the struct and eliminate one layer of indirection.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rocketmq-common/src/common/filter.rsrocketmq-common/src/common/filter/op.rsrocketmq-common/src/common/filter/operand.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rocketmq-common/src/common/filter/op.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-common/src/common/filter/operand.rs (1)
rocketmq-common/src/common/filter/op.rs (3)
new(24-26)symbol(15-15)symbol(27-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: auto-approve
🔇 Additional comments (2)
rocketmq-common/src/common/filter/operand.rs (1)
30-34: LGTM!The
Optrait implementation correctly delegates to the innerOpBase, following the expected delegation pattern.rocketmq-common/src/common/filter.rs (1)
17-18: No action needed – module changes are correct.The addition of
pub mod op;andpub mod operand;is correct, and thefilter_typemodule remains available for internal use. No code in the codebase importsfilter_typeas a public module, so unexporting it does not cause any breakage.
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
4 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #5410
Fixes #5411
Brief Description
Added Op and Operand by converting from java to rust and placed them in filter
How Did You Test This Change?
wrote unit tests for them
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.