Skip to content
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

feat: simple AST for search #1175

Merged
merged 6 commits into from May 4, 2023
Merged

feat: simple AST for search #1175

merged 6 commits into from May 4, 2023

Conversation

dranikpg
Copy link
Contributor

@dranikpg dranikpg commented May 2, 2023

Simplest possible AST for: term, not, and, or

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@chakaz chakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to disregard any comments you don't like!

src/core/search/ast_expr.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Show resolved Hide resolved
src/core/search/ast_expr.h Show resolved Hide resolved
src/core/search/query_driver.h Outdated Show resolved Hide resolved
src/core/search/ast_expr.h Outdated Show resolved Hide resolved
@dranikpg dranikpg force-pushed the parser-ast branch 2 times, most recently from cc26d47 to c398dd8 Compare May 3, 2023 14:34
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg
Copy link
Contributor Author

dranikpg commented May 3, 2023

@romange @chakaz

Please check the parser file, I'm not sure I'm getting precedence right. It currently prints only three ambiguity warnings connected with field terms - I don't parse them anyways yet

@dranikpg dranikpg marked this pull request as ready for review May 3, 2023 15:02
@romange
Copy link
Collaborator

romange commented May 3, 2023

I hope @chakaz will be able to help here.

@chakaz
Copy link
Collaborator

chakaz commented May 3, 2023

I'm afraid I've never done anything related to lex, so I'm useless on that area. Perhaps ChatGPT has some experience? :|

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg
Copy link
Contributor Author

dranikpg commented May 4, 2023

I'm afraid I've never done anything related to lex

Me neither 🙂 But its fairly simple so far

Perhaps ChatGPT has some experience? :|

I asked it, but its not an expert there. Though you can ask it about how to do smth (for example setting precedence for operator without a real token)

I've added some more tests to check for correctness

Co-authored-by: Roy Jacobson <roi.jacobson1@gmail.com>
Signed-off-by: Vladislav <vladislav.oleshko@gmail.com>
@dranikpg dranikpg requested a review from chakaz May 4, 2023 08:08
@@ -58,6 +75,18 @@ class SearchParserTest : public ::testing::Test {
ASSERT_TRUE(caught); \
}

#define CHECK_ALL(...) \
{ \
for (auto input : {__VA_ARGS__}) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but I'd prefer to see separate check statements for each input in the test itself. I would be easier to debug as well (we get the line number, and no macros to manually unroll :)
You could, if you want to, add a custom matcher to print the DebugExpr(), but I actually think that it's not as useful if we remove the macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line number is preserved, that's why I use a macro. You have all the info you need.

DebugExpr(), but I actually think that it's not as useful if we remove the macros.

DebugExpr is always useful because it shows you the tree built by the parser

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
Copy link
Contributor

@royjacobson royjacobson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dranikpg dranikpg merged commit 957fe83 into dragonflydb:main May 4, 2023
6 checks passed
@dranikpg dranikpg deleted the parser-ast branch May 28, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants