-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
First Implementation of GQL parser #11
Conversation
jenkins please |
Can one of the admins verify this patch? |
jenkins please |
Build failed. |
1 similar comment
Build failed. |
Build failed. |
Build succeeded. |
Build succeeded. |
Awesome work! I'll take a look this morning on the train |
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.
Highly suggest to separate AstTypes into multiple files
@@ -0,0 +1,1148 @@ | |||
/* Copyright (c) 2018 - present, VE Software Inc. All rights reserved |
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.
I would suggest to separate this file into multiple smaller files
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.
Yes, this makes sense. But I am working on the INSERT
and UPDATE
syntax, I will get this done after that.
Build succeeded. |
1 similar comment
Build succeeded. |
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.
Well done!! LGTM
Please address the minor comments before committing
TEST(Parser, Go) { | ||
{ | ||
GQLParser parser; | ||
std::string query = "GO FROM 1 AS person"; |
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.
I understand this is the very early version. With more code added, we might want to check the correctness of the parsing results. It would be helpful to have a TODO comment here to remind ourselves
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.
Of course, later we shall verify the validness of the parsing tree by inspecting the internal structures.
parser/scanner.lex
Outdated
{ID} { yylval->strval = new std::string(yytext, yyleng); return TokenType::SYMBOL; } | ||
|
||
[0-9]+ { yylval->intval = ::atoll(yytext); return TokenType::INTEGER; } | ||
[0-9]+[Uu][Ll]? { yylval->intval = ::atoll(yytext); return TokenType::UINTEGER; } |
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.
I don't think we need to support UInt. We will treat all integers long, so we don't need ""[Uu]" and "[Ll]" here
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 seems an open question, since there would be some annoying issues without the support of unsigned integers. We could discuss about this later。
%parse-param { vesoft::Statement** statement } | ||
|
||
%code requires { | ||
#include <iostream> |
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.
You can use Cord instead of sstream :-)
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.
Actually, I haven't grasped the use occasion of Cord
. Besides, I intend to see if we could feed a normal string buffer to the scanner.
parser/AstTypes.h
Outdated
virtual std::string toString() const { | ||
return ""; | ||
} | ||
using ReturnType = boost::variant<int64_t, uint64_t, double, bool, std::string>; |
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.
Styling: Would be nice to have a blank line above
parser/AstTypes.h
Outdated
static int64_t asInt(const ReturnType &value) { | ||
return boost::get<int64_t>(value); | ||
} | ||
static uint64_t asUInt(const ReturnType &value) { |
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.
I don't think we need to support UInt. Objection?
In addition, added support for hexadecimal and octal numbers.
Build succeeded. |
1 similar comment
Build succeeded. |
Build succeeded. |
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 awesome!!
* First Implementation of GQL parser * Address @sherman-the-tank 's comments * Implement syntax of UPDATE/INSERT In addition, added support for hexadecimal and octal numbers. * Reserve space on the buffer in `toString`
* First Implementation of GQL parser * Address @sherman-the-tank 's comments * Implement syntax of UPDATE/INSERT In addition, added support for hexadecimal and octal numbers. * Reserve space on the buffer in `toString`
Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com>
No description provided.