Add support for prepared statements. #229

reltuk · 2020-11-06T21:12:15Z

No description provided.

CLAassistant · 2020-11-06T21:12:20Z

All committers have signed the CLA.

reltuk · 2020-11-06T21:21:39Z

@zachmu Looking for a quick pass and any thoughts here. Approach:

In StmtPrepare, give the query to the Analyzer and return the Schema() fields from the fully analyzed query.
In StmtExecute, give the query to the parser, replace all the expression nodes of type BindVar with expression.NewLiteral nodes for the incoming bindings, give the resulting parse tree to the Analyzer, run the query as normal. Currently we re-analyze the queries because the types of the replaced expression nodes might affect analysis, and those can change from Execute to Execute.
bindingsToSql is a little bit sad, but I think the approach is necessary and reasonable. For the most part, chooses to create the widest possible type instances of the incoming values.

Currently think that testing strategy here is going to be:

Add some parse / plan tests for new BindVar nodes.
Add some unit tests for ApplyBindings.
Add some engine tests that use QueryWithBindings, probably along the lines of existing Query, Delete, Update, Insert tests.
When we update Dolt to pick up these changes, add some cases to existing client connector tests to hit prepared statements as well.

Does that seem reasonable?

zachmu

Looks like a good approach!

zachmu · 2020-11-06T23:07:30Z

sql/parse/parse.go

@@ -1786,7 +1786,7 @@ func convertVal(v *sqlparser.SQLVal) (sql.Expression, error) {
 		}
 		return expression.NewLiteral(val, sql.LongBlob), nil
 	case sqlparser.ValArg:
-		return expression.NewLiteral(string(v.Val), sql.LongText), nil
+		return expression.NewBindVar(strings.TrimPrefix(string(v.Val), ":")), nil


Do named bindvars need to be distinct? If so, need an analyzer func to check that

No, they're also support for a different syntax, SELECT * FROM test WHERE id = :id, and the identifier id could be used multiple times in the query. IDs are auto-assigned by the tokenizer for the ? syntax though, and those are distinct.

There's a problem though. The parser will parse a query that uses both ? and :name style variables and with the current approach it will run into a conflict if someone names one of their vars :v1, for example. Longer-term we should distinguish between the two types of variables in the parser I think.

Should have a few engine tests for the named var bindings in that case

Including tests of using the same named binvar multiple places in the same query, and trying to execute with named bindvars that aren't in the original query (I can't remember seeing tests of the latter, definitely need engine tests of the former)

zachmu · 2020-11-06T23:37:22Z

server/handler.go

+		if err != nil {
+			return nil, err
+		}
+		switch {


Seems like these would be better expressed as a switch on v.Type(), no?

Yeah, maybe...I was using a few expressions like sqltypes.IsUnsigned(v.Type()), so I went with this approach...could change it or split it into two groups...

…ort and handler bindingsToSql.

zachmu

Generally looks good, got a couple requests for more tests and a very annoying request to not add a nil struct param to every QueryTest struct, sorry.

I don't think we need to support subqueries in v1.

enginetest/queries.go

server/handler.go

zachmu · 2020-11-10T22:54:13Z

sql/parse/parse.go

@@ -1786,7 +1786,7 @@ func convertVal(v *sqlparser.SQLVal) (sql.Expression, error) {
 		}
 		return expression.NewLiteral(val, sql.LongBlob), nil
 	case sqlparser.ValArg:
-		return expression.NewLiteral(string(v.Val), sql.LongText), nil
+		return expression.NewBindVar(strings.TrimPrefix(string(v.Val), ":")), nil


Should have a few engine tests for the named var bindings in that case

zachmu · 2020-11-10T22:57:21Z

sql/parse/parse.go

@@ -1786,7 +1786,7 @@ func convertVal(v *sqlparser.SQLVal) (sql.Expression, error) {
 		}
 		return expression.NewLiteral(val, sql.LongBlob), nil
 	case sqlparser.ValArg:
-		return expression.NewLiteral(string(v.Val), sql.LongText), nil
+		return expression.NewBindVar(strings.TrimPrefix(string(v.Val), ":")), nil


Including tests of using the same named binvar multiple places in the same query, and trying to execute with named bindvars that aren't in the original query (I can't remember seeing tests of the latter, definitely need engine tests of the former)

sql/plan/bindvar.go

sql/plan/bindvar_test.go

…k across subqueries.

…red-statements

zachmu

Still don't see an engine test for applying the same named bindvar more than once in a query. Otherwise LGTM!

reltuk requested a review from zachmu November 6, 2020 21:12

Add support for prepared statements.

0a0af60

reltuk force-pushed the aaron/support-prepared-statements branch from 1f4ec38 to 0a0af60 Compare November 6, 2020 21:13

zachmu reviewed Nov 6, 2020

View reviewed changes

reltuk added 2 commits November 10, 2020 13:36

Add some prepared statement tests for plan.ApplyBindings, parser supp…

460e263

…ort and handler bindingsToSql.

enginetest: Adding tests for simple prepared statement bindings.

ff97c61

zachmu approved these changes Nov 10, 2020

View reviewed changes

reltuk added 5 commits November 10, 2020 15:47

sql/plan/bindvar.go: Make prepared statement binding substitution wor…

27ce4e8

…k across subqueries.

PR feedback.

371c8dc

enginetest: Convert query tests to named field struct literals.

049e056

enginetest: Drop Bindings: nil in query test struct literals.

e576527

Merge remote-tracking branch 'origin/master' into aaron/support-prepa…

4990f6e

…red-statements

reltuk marked this pull request as ready for review November 11, 2020 00:51

zachmu approved these changes Nov 11, 2020

View reviewed changes

enginetest: Add test for same BindVar used multiple times.

0989e34

reltuk merged commit d00e294 into master Nov 11, 2020

Hydrocharged deleted the aaron/support-prepared-statements branch December 3, 2020 10:01

Uh oh!

Add support for prepared statements. #229

Add support for prepared statements. #229

Uh oh!

Conversation

reltuk commented Nov 6, 2020

Uh oh!

CLAassistant commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reltuk commented Nov 6, 2020

Uh oh!

zachmu 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!

zachmu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zachmu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CLAassistant commented Nov 6, 2020 •

edited

Loading