Support ON/OFF for SET autocommit by kuba-- · Pull Request #383 · src-d/go-mysql-server

kuba-- · 2018-09-26T11:58:43Z

Signed-off-by: kuba-- kuba@sourced.tech
It closes #374

kuba-- · 2018-09-27T15:36:19Z

Finally it compiles after upgrading vitess.

erizocosmico · 2018-09-27T15:37:28Z

sql/parse/parse.go

-		default:
-			return nil, ErrUnsupportedFeature.New("qualifiers in set variable names other than @@session")
+		name := strings.TrimSpace(e.Name.Lowered())
+		if strings.Contains(name, "autocommit") {


Is ON/OFF only for autocommit on MySQL?

Maybe there are different cases (I didn't investigate any other cases, just focused on this particular case), but the main issue here can be transformation. For some expressions Type() panics.
I'm totally open to have the most generic solution transformation here.

kuba-- · 2018-09-27T16:17:21Z

@erizocosmico - I've made ON/OFF more generic (no autocommit checks). I just added extra if

if !e.Resolved() || e.Type() != sql.Text

in transformation and looks that it works.

ajnavarro

Good!

Some minor changes related with the makefile script that is already on another PR and questions about variables.

ajnavarro · 2018-09-28T07:19:26Z

_scripts/go-vitess/Makefile

@@ -1,56 +1,41 @@
-# Tooling to create the package `gopkg.in/src-d/go-vitess.v0`.
+# Tooling to create the package `gopkg.in/src-d/go-vitess.v1`.


I would prefer to have this script separated in another PR as you did: #384

yeah, it's already there
#384. I just didn't revert changes on this branch (so doesn't matter in what order we'll merge it, hopefully there will not be conflicts).

#384 is already merged. This file shouldn't be here now.

ajnavarro · 2018-09-28T07:22:04Z

engine_test.go

 	ctx := sql.NewContext(context.Background(), sql.WithSession(session), sql.WithPid(1))

-	_, _, err := e.Query(ctx, `set autocommit=1, sql_mode = concat(@@sql_mode,',STRICT_TRANS_TABLES')`)
+	_, _, err := e.Query(ctx, `set autocommit=ON, sql_mode = concat(@@sql_mode,',STRICT_TRANS_TABLES')`)


autocommit=1 should work too, so I would prefer to do not change this test.

ok, I'll add another test.

ajnavarro · 2018-09-28T07:22:28Z

go.mod

@@ -1,65 +1,19 @@
 module gopkg.in/src-d/go-mysql-server.v0

 require (


Is it normal that all that dependencies disappear?

I'm not sure how vgo builds this dependency graph, but as you see everything what disappeared are indirect deps. The good thing is that it still builds.

ajnavarro · 2018-09-28T07:25:28Z

sql/parse/parse_test.go

 	`SET @@session.autocommit=1, foo="bar"`: plan.NewSet(
 		plan.SetVariable{
-			Name:  "autocommit",
+			Name:  "@@session.autocommit",


this means that we are adding session variables as global variables (but with a name @@session.blah) now?

Why the change adding @@session.? It should work without it too

Because right now for SetExpr we don't have Qualifiers so in convertSet we return what was passed.

But we should trim that part as it was before. Only session variables should be saved with Set

ajnavarro · 2018-09-28T07:28:11Z

sql/session.go

 	s.mu.Lock()
 	defer s.mu.Unlock()
+
+	key = strings.TrimLeft(key, "@")


so, all variables are set as session level? what happens when the user disconnects? per example autocommit=1 will disappear?

autocommit is a session variable, isn't it?

Here what I only changed is checking if we have session prefix and remove them before set. Because right now we don't have Qualifiers in SetExpr, so if you try sth, like:

SET @autocommit = 1 SELECT @@session.autocommit

we'll set in s.config["autocommit"]=1 but for select we'll try to get s.config["session.autocommit"] what is not there.

that's why the session. should be removed in the select

ok, makes sense. I've moved that logic from session.Get/Set to
analyzer.resolveColumns and to plan.(*Set)RowIter

I had to fix TestResolveColumnsSession. I think right now it wasn't right (correct me if I'm wrong):
it looked like:

node := plan.NewProject( []sql.Expression{ expression.NewUnresolvedColumn("@@foo_bar"), expression.NewUnresolvedColumn("@@bar_baz"), }, plan.NewResolvedTable(dualTable), ) result, err := resolveColumns(ctx, NewDefault(nil), node) require.NoError(err) expected := plan.NewProject( []sql.Expression{ expression.NewGetSessionField("@@foo_bar", sql.Int64, int64(42)), // <-- @@- expression.NewGetSessionField("@@bar_baz", sql.Null, nil), // <-- @@ }, plan.NewResolvedTable(dualTable), )

I assumed that in session we don't keep fields prefixed by @, because:

if you take a look into session config:

func defaultSessionConfig() map[string]typedValue { return map[string]typedValue{ "auto_increment_increment": typedValue{Int64, int64(1)}, "time_zone": typedValue{Text, time.Local.String()}, "system_time_zone": typedValue{Text, time.Local.String()}, "max_allowed_packet": typedValue{Int32, math.MaxInt32}, "sql_mode": typedValue{Text, ""}, } }

and also func (f *GetSessionField) String() string { return "@@" + f.name } already prefixes with "@".

ajnavarro · 2018-09-28T09:24:25Z

@kuba-- could you rebase please?

kuba-- · 2018-09-28T09:26:10Z

@ajnavarro - done.

kuba-- · 2018-09-28T09:39:45Z

@ajnavarro - hmm I've rebased again and I don't see Makefile in changes.

ajnavarro · 2018-09-28T09:40:38Z

sql/session.go

 func (s *BaseSession) Set(key string, typ Type, value interface{}) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
+


that file shouldn't be modified on that PR.

ajnavarro

just session.go minor change and LGTM

ajnavarro · 2018-09-28T09:45:02Z

hmm, we are testing with session columns, and that line appears that never is true: https://codecov.io/gh/src-d/go-mysql-server/compare/7f8e182c469e77663f886baa8170a5662733d185...645a95d0855995f7b623c4d4b862e835478d57e4/src/sql/analyzer/resolve_columns.go#L71

Signed-off-by: kuba-- <kuba@sourced.tech> Add more parser tests. Handle also true/false. Signed-off-by: kuba-- <kuba@sourced.tech>

kuba-- · 2018-09-28T10:39:23Z

@ajnavarro - I've added many tests (even to test this particular line), but I'm not sure why this cover tool still complains.

jfontan · 2018-09-28T10:42:02Z

@kuba-- I don't know if this is the case but coverage by default only counts coverage in files from the same package as the test. If that code path is used with a test from another package it's not counted.

kuba-- · 2018-09-28T10:47:26Z

@jfontan - I think it's not the case. I extended TestQualifyColumns to test qualifyColumns and it still complain on the same line, and everything is in the same package.

kuba-- added enhancement New feature or request proposal proposal for new additions or changes wip work in progress and removed wip work in progress labels Sep 26, 2018

kuba-- changed the title ~~[WIP] Support ON/OFF for SET autocommit~~ Support ON/OFF for SET autocommit Sep 27, 2018

kuba-- requested review from a team and erizocosmico September 27, 2018 15:35

kuba-- requested a review from ajnavarro September 27, 2018 15:36

erizocosmico reviewed Sep 27, 2018

View reviewed changes

ajnavarro suggested changes Sep 28, 2018

View reviewed changes

ajnavarro reviewed Sep 28, 2018

View reviewed changes

ajnavarro approved these changes Sep 28, 2018

View reviewed changes

erizocosmico approved these changes Sep 28, 2018

View reviewed changes

Support ON/OFF for SET autocommit

84d5bd9

Signed-off-by: kuba-- <kuba@sourced.tech> Add more parser tests. Handle also true/false. Signed-off-by: kuba-- <kuba@sourced.tech>

jfontan approved these changes Sep 28, 2018

View reviewed changes

ajnavarro merged commit a086daa into src-d:master Sep 28, 2018

kuba-- deleted the fix-374/upgrade-vitess branch September 28, 2018 11:58

		@@ -1,56 +1,41 @@
		# Tooling to create the package `gopkg.in/src-d/go-vitess.v0`.
		# Tooling to create the package `gopkg.in/src-d/go-vitess.v1`.

		@@ -1,65 +1,19 @@
		module gopkg.in/src-d/go-mysql-server.v0

		require (

Conversation

kuba-- commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuba-- commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuba-- commented Sep 27, 2018

Uh oh!

ajnavarro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuba-- Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuba-- Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajnavarro commented Sep 28, 2018

Uh oh!

kuba-- commented Sep 28, 2018

Uh oh!

kuba-- commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajnavarro left a comment

Choose a reason for hiding this comment

Uh oh!

ajnavarro commented Sep 28, 2018

Uh oh!

kuba-- commented Sep 28, 2018

Uh oh!

jfontan commented Sep 28, 2018

Uh oh!

kuba-- commented Sep 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kuba-- commented Sep 26, 2018 •

edited

Loading

kuba-- Sep 28, 2018 •

edited

Loading

kuba-- Sep 28, 2018 •

edited

Loading