- 
                Notifications
    
You must be signed in to change notification settings  - Fork 101
 
fix(datadog search): Fix improper handling of conjunctions by the datadog query parser #1542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
1541953    to
    e82fc30      
    Compare
  
    | 
           Before reviewing, I would argue that, as written above, this is a breaking change, as it could make the behavior of existing queries change.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does look like a definite fix and good simplification of the logic. It does have me wondering if it would need to be something recursive, however, to fit all possible cases of nested parentheses. Could you comment on that?
| Rule::PLUS => (), | ||
| Rule::NOT => { | ||
| modifier = Some(LuceneOccur::MustNot); | ||
| is_not = true; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to toggle? i.e. is_not = !is_not;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like, a query containing "NOT NOT"? I just checked, the lexer does not allow it (it fails on "NOT -foo" and interprets NOT NOT foo as NOT "NOT" foo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That still makes me nervous, given the prevalence of double-negation in other such expression parsers, but it's good to know it's at least not wrong.
          
 Parentheses are interpreted by the lexer as the beginning of a sub-clause. The parser is already recursive (visit_query -> visit_clause -> visit_query)  | 
    
| 
           @pront could you review this as well?  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for improving the tests.
Summary
The Datadog query parser incorrectly interprets queries mixing AND and OR statements.
For example:
This PR fixes this behavior, while simplifying the parser logic.
Change Type
Is this a breaking change?
How did you test this PR?
Added and updated unit tests
Does this PR include user facing changes?
our guidelines.
Checklist
run
dd-rust-license-tool writeand commit the changes. More details here.References