Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

For "set @@session.autocommit" replace on/1, off/0. #381

Closed
wants to merge 1 commit into from
Closed

For "set @@session.autocommit" replace on/1, off/0. #381

wants to merge 1 commit into from

Conversation

kuba--
Copy link
Contributor

@kuba-- kuba-- commented Sep 24, 2018

Signed-off-by: kuba-- kuba@sourced.tech
This is workaround which for queries SET @@session.autocommitreplaces ON by 1 and OFF by 0.
At least it fixes #374

screen shot 2018-09-24 at 22 55 24

Signed-off-by: kuba-- <kuba@sourced.tech>
@kuba--
Copy link
Contributor Author

kuba-- commented Sep 24, 2018

@src-d/data-retrieval mysql tests are failing due to:

    --- FAIL: TestDescribe/parallel (0.00s)
        --- FAIL: TestDescribe/parallel/DESCRIBE_FORMAT=TREE_SELECT_*_FROM_mytable (0.00s)
            require.go:97: 
                	Error Trace:	engine_test.go:711
                	Error:      	lengths don't match: 4 != 3
                	Test:       	TestDescribe/parallel/DESCRIBE_FORMAT=TREE_SELECT_*_FROM_mytable

So this PR also is failing (because it was branched from the master).

@@ -35,6 +35,7 @@ var (
showIndexRegex = regexp.MustCompile(`^show\s+(index|indexes|keys)\s+(from|in)\s+\S+\s*`)
describeRegex = regexp.MustCompile(`^(describe|desc|explain)\s+(.*)\s+`)
fullProcessListRegex = regexp.MustCompile(`^show\s+(full\s+)?processlist$`)
setAutocommitRegexp = regexp.MustCompile(`^set\s+(@+)session.autocommit\s*=`)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is already support for set in go-mysql-server, there is no need to add a regexp for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but because ON is a keyword, yacc Parse fails. That's why I first converts on/off to 1/0 and pass through regular workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe we can pre-process the query and replace the on/off with 1/0 before actually parsing it using sqlparser. There might be other queries that use on/off, this is very ad-hoc.

Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

I would prefer a less hacky and ad-oc solution. Did you try using the parser? maybe that ON OFF is supported.

@kuba--
Copy link
Contributor Author

kuba-- commented Sep 25, 2018

I'm closing this one. Hopefully we can get it for free after upgrade vitess to v2.2

@kuba-- kuba-- closed this Sep 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ON/OFF as synonyms of 1/0
3 participants