Refactor parse #728
Refactor parse #728
Conversation
parse/grammar/KuneiformParser.g4
Outdated
| ; | ||
|
|
||
| sql_expr: | ||
| literal type_cast? # literal_sql_expr |
There was a problem hiding this comment.
It would be really clear and easier to read if the 'alternate label' are aligned on the right. I kind of have a strong feeling about it, especially when I'm reviewing it.
There was a problem hiding this comment.
Yeah that's fair. Will do that
There was a problem hiding this comment.
cool, can do do it on all rules? That'll be super helpful
There was a problem hiding this comment.
I've reviewed the grammar rules till sql_expr, and corresponded parser logic, I kind go over the parser line by line since we've change the grammer so much. Will continue to review.
BTW, I like the new grammer/visitor/tree structure, a lot cleaner, and it helps a lot since we have matching structures.
It's much easier than the version before for reviewing, @jchappelow.
| LEGACY_NO_ACTION: 'no_action'; | ||
|
|
||
| IDENTIFIER: | ||
| [a-z] [a-z_0-9]* |
There was a problem hiding this comment.
For SQL this might be a issue, we used to support table/column name surrounded by ` and ". Unless we want to remove this support.
parse/grammar/KuneiformParser.g4
Outdated
|
|
||
| constraint: | ||
| MIN LPAREN literal RPAREN # MIN | ||
| | MAX LPAREN literal RPAREN # MAX |
There was a problem hiding this comment.
Seems nbd. You probably did this on purpose.
I don't think you mean to use keywords from Lexer(I don't think you can? because ANTLR spits out just the captialized label) for 'alternate label', but here it's bit confusing in IDE
There was a problem hiding this comment.
oh whoops yeah that's really dumb of me
| LBRACE action_block RBRACE | ||
| ; | ||
|
|
||
| procedure_declaration: |
There was a problem hiding this comment.
Just came up a edge case: If a view procedure calls another view(authn=true) procedure, it bypasses the called procedure's authn constraint, but this combination seems won't happen.
I guess the author should be aware that the caller procedure also needs to be view(authn=true)
There was a problem hiding this comment.
Yeah Truflation ran into this. Not really something we can handle in the parser. But yeah, it makes me think we need some better access control.
There was a problem hiding this comment.
not sure how we can ensure this, since we cannot enforce the caller procedure has same annotation as called procedure(or only enforce authn annotation), it's not kwild's concern
|
|
||
| func (s *schemaVisitor) VisitStmt_variable_declaration(ctx *gen.Stmt_variable_declarationContext) any { | ||
| stmt := &ProcedureStmtDeclaration{ | ||
| Variable: varFromTerminalNode(ctx.VARIABLE()), |
parse/grammar/KuneiformParser.g4
Outdated
| | RETURN NEXT procedure_expr_list SCOL # stmt_return_next | ||
| ; | ||
|
|
||
| // unfortunately necessary to preserve order in generated code |
There was a problem hiding this comment.
I'd prefer to not have to declared a rule this basic that is only used once, but it is necessary to know the order that these are used in. Will delete the comment
parse/antlr.go
Outdated
| act.Public = public | ||
| act.Modifiers = mods | ||
|
|
||
| params := ctx.Variable_list().Accept(s).([]*ExpressionVariable) |
| paramStrs[i] = p.String() | ||
| } | ||
| act.Parameters = paramStrs | ||
|
|
There was a problem hiding this comment.
call ctx.Action_block().Accept(s) here?
There was a problem hiding this comment.
Ah yeah we dont' need this, as we'll parse this from ParseAction
There was a problem hiding this comment.
We do actually, to preserve order. Fixed
There was a problem hiding this comment.
What order? will we compare the result later?
parse/grammar/KuneiformParser.g4
Outdated
| (ELSE else_clause=sql_expr)? END #case_expr | ||
| | (NOT? EXISTS)? LPAREN select_statement RPAREN type_cast? # subquery_sql_expr | ||
| // setting precedence for arithmetic operations: | ||
| | left=sql_expr CONCAT right=sql_expr # arithmetic_sql_expr |
There was a problem hiding this comment.
It feels strange to treat concat operator as a arithmetic operator, I know it's for the convenience of parser/analyzer, but this will cause confusion IMO.
There was a problem hiding this comment.
Yeah I just didn't know where else to include
This commit unifies all of the parsers into a single grammar. This is so that we can get accurate error messages in the IDE.
This commit also introduces uint256 and decimal types.
Still a WIP, as I still need to: