-
Notifications
You must be signed in to change notification settings - Fork 208
fix(parser): Allow parenthesis in query and filter terms #788
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
fix(parser): Allow parenthesis in query and filter terms #788
Conversation
This comment has been minimized.
This comment has been minimized.
WalkthroughExternalized and refined tokenization for parentheses, words, OR, and negation; updated LR parser state/token tables and term name mappings; grammar now uses external tokens for ParenExpr; added extensive parenthesis parsing tests and adjusted OR-related expectations. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as InputStream
participant Tokenizers as Tokenizer chain (tokens.ts)
participant Parser as LRParser (parser.ts)
participant AST as AST builder / semantic actions
Input->>Tokenizers: read characters
Tokenizers->>Tokenizers: apply rules (balanced-paren, prefixes, OR/negation)
Tokenizers-->>Parser: emit tokens (openParen, word, closeParen, or, negate, ...)
Parser->>Parser: consult states / tokenData / goto
Parser-->>AST: invoke reductions → build Program / Expr nodes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
brendan-kellam
left a comment
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.
lgtm
The parser was previously not allowing words to include parenthesis. This is a problem because it caused queries like
test(to fail when in reality this is valid. It also caused issues like #771 when parenthesis were present in filter expressions as well.This PR fixes that by modifying the lexer/parser do allow words to include parenthesis. The reason this is really complicated is because parenthesis are picked up to detect groupings (i.e. in queries like
(foo bar)). Lexers don't like this because it requires them to look ahead to determine what the token produced should be. For examples should(foo bar)be a ParenExpr or two words(fooandbar)?Needless to say this took a lot of prompting with cursor and claude to arrive at a working solution. Thankfully we have an extensive test suite so we can be pretty sure that no existing functionality has broken
Also fixes #771
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.