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

Added CREATE PROCEDURE & FUNCTION #46

Merged
merged 1 commit into from Feb 16, 2021
Merged

Conversation

Hydrocharged
Copy link

@Hydrocharged Hydrocharged commented Feb 12, 2021

This adds:

CREATE PROCEDURE
SHOW PROCEDURE STATUS
SHOW FUNCTION STATUS
DROP PROCEDURE
CALL

The SHOW...CODE statements are being skipped for now, as they're relatively unused compared to the above statements. Additionally, the CREATE statements do not have a fully fleshed-out routine body, as we currently do not parse all valid statements anyway.

@Hydrocharged
Copy link
Author

Hydrocharged commented Feb 12, 2021

The conflicts: 24 shift/reduce is not something that I know how to fix, nor am I too sure what's going on. Googling shows it deals with precedence, but it wasn't helping out too much. If it's deemed critical I can spend more time trying to fix it.
Removed CREATE FUNCTION for now to remove this issue.

@zachmu
Copy link
Member

zachmu commented Feb 12, 2021

Sadly you do need to deal with the shift/reduce conflicts. They're a pain, but you can't actually guarantee a correct parse unless you eliminate them.

The way to debug them is to examine the .output file when you run goyacc.

goyacc -o sql.go sql.y

Produces a log called y.output. If you search it for shift/reduce you can get clues on where the conflict is coming from.

These conflicts happen when the grammar has ambiguity, so when there is a place in the input where two different different rules could be applied next. Can't have it. The most common approach to eliminating these is to combine multiple rules into one to eliminate the ambiguity, which you can see was done various places in the grammar (read the comments).

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

This all looks pretty reasonable, nice work. I'll take another look when you sort out the conflicts in the grammar.

@@ -2080,9 +2310,21 @@ show_statement:
{
$$ = &Show{Type: string($2)}
}
| SHOW PROCEDURE ddl_skip_to_end
| SHOW PROCEDURE CODE ddl_skip_to_end
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually want the skips on these? Typically those are for unsupported / semi-supported features

Copy link
Author

Choose a reason for hiding this comment

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

We don't support the CODE ones for now, which are different than the STATUS ones. We probably won't ever support the CODE ones tbh. They return something that looks like some kind of MySQL IL.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM!

go/vt/sqlparser/ast.go Show resolved Hide resolved
go/vt/sqlparser/ast.go Outdated Show resolved Hide resolved
go/vt/sqlparser/ast.go Outdated Show resolved Hide resolved
go/vt/sqlparser/ast.go Show resolved Hide resolved
go/vt/sqlparser/ast.go Show resolved Hide resolved
go/vt/sqlparser/parse_test.go Outdated Show resolved Hide resolved
@Hydrocharged Hydrocharged merged commit 78bbab1 into master Feb 16, 2021
@Hydrocharged Hydrocharged deleted the daylon/procedures branch February 16, 2021 10:26
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

2 participants