feat: handle AND/OR and priority in matches fn #4270
Conversation
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis update implements significant changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant MatchesFunction as MatchesFunction
participant PatternAst as PatternAst
participant ASTTransformer as ASTTransformer
User->>MatchesFunction: Execute match function
MatchesFunction->>PatternAst: Parse Pattern Expression
PatternAst->>ASTTransformer: Initiate transformation process
ASTTransformer-->>PatternAst: Return transformed AST
PatternAst-->>MatchesFunction: Transformed AST
MatchesFunction-->>User: Evaluate and return results
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Signed-off-by: Ruihang Xia <[email protected]>
|
@coderabbitai pause |
![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)
src/common/function/src/scalars/matches.rs (1)
Line range hint
15-18: Potential issue with the recursive call ineval.The recursive call to
self.evalin theevalfunction might cause a stack overflow if the pattern is too complex. Consider refactoring this part to avoid deep recursion.- self.eval(columns[0].clone(), pattern) + self.evaluate(columns[0].clone(), pattern)fn evaluate(&self, data: VectorRef, pattern: String) -> Result<VectorRef> { // existing implementation }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/common/function/src/lib.rs (1 hunks)
- src/common/function/src/scalars/matches.rs (12 hunks)
Files skipped from review due to trivial changes (1)
- src/common/function/src/lib.rs
Additional comments not posted (19)
src/common/function/src/scalars/matches.rs (19)
165-185: LGTM!The function
into_like_exprcorrectly handles the new AST structure and the use of thereducemethod is appropriate.
209-219: LGTM!The function
transform_astcorrectly applies the transformation rules and the use of thetransform_upandtransform_downmethods is appropriate.
226-266: LGTM!The function
collapse_binary_branch_fncorrectly collapses binary branches with the same operator and the use of theTransformedenum is appropriate.
273-339: LGTM!The function
eliminate_optional_fncorrectly eliminates optional patterns based on the specified rules.
346-364: LGTM!The function
eliminate_single_child_fncorrectly eliminates single child nodes.
369-384: LGTM!The function
apply_childrencorrectly implements theapply_childrenmethod of theTreeNodetrait forPatternAst.
387-402: LGTM!The function
map_childrencorrectly implements themap_childrenmethod of theTreeNodetrait forPatternAst.
Line range hint
424-435: LGTM!The function
parse_patterncorrectly handles the new AST structure and the use of theto_rpnmethod is appropriate.
147-150: LGTM!The enum variant
PatternAst::Binarycorrectly includes children for binary operations.
151-157: LGTM!The enum variant
UnaryOpcorrectly represents unary operators.
158-164: LGTM!The enum variant
BinaryOpcorrectly represents binary operators.
Line range hint
784-805: LGTM!The enum variant
Tokencorrectly includes new token types for the parser.
Line range hint
137-146: LGTM!The enum variant
PatternAstcorrectly includes new variants for pattern matching.
Line range hint
165-402: LGTM!The struct implementation
PatternAstcorrectly includes new methods for pattern matching.
Line range hint
424-435: LGTM!The struct implementation
ParserContextcorrectly handles the new AST structure.
Line range hint
784-805: LGTM!The test function
valid_matches_tokenizercorrectly includes new test cases for the tokenizer.
Line range hint
784-805: LGTM!The test function
invalid_matches_tokenizercorrectly includes new test cases for invalid patterns.
Line range hint
784-805: LGTM!The test function
valid_ast_transformercorrectly tests the AST transformer with various cases.
Line range hint
784-805: LGTM!The test function
valid_matches_parsercorrectly includes new test cases for the parser.
Actions performedReviews paused. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4270 +/- ##
==========================================
- Coverage 84.94% 84.68% -0.27%
==========================================
Files 1057 1057
Lines 187517 187814 +297
==========================================
- Hits 159279 159041 -238
- Misses 28238 28773 +535 |
quote them should fine. I'll add a test case for this. |
Signed-off-by: Ruihang Xia <[email protected]>
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
The A in AST is for abstract. So does this patch
Checklist
Summary by CodeRabbit
New Features
Refactor