vtgate: subquery support by sougou · Pull Request #3923 · vitessio/vitess

sougou · 2018-05-12T04:32:23Z

This implementation follows the design from doc/VTGateSubqueries.md.

A new PulloutSubquery primitive has been created. Its action is to
execute a subquery, save the resulting value(s) in a bind variable,
and pass that down to the underlying primitive.

The type of value set depends on whether the subquery occurred in
an IN clause, etc. In the case of IN and NOT IN clauses, if the
list returned is empty, a guard variable is set to work around
the inability for IN clauses to handle an empty list syntax.

Subqueries can only be pulled out if they are not correlated.
Correlated subqueries will be handled when we add the
ability to execute expressions in VTGate.

derekperkins · 2018-10-15T14:54:25Z

@sougou Is this complete and just waiting on a review?

sougou · 2018-10-15T14:58:45Z

Pretty much. I need to update to the latest master.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

This implementation follows the design from doc/VTGateSubqueries.md. A new PulloutSubquery primitive has been created. Its action is to execute a subquery, save the resulting value(s) in a bind variable, and pass that down to the underlying primitive. The type of value set depends on whether the subquery occurred in an IN clause, etc. In the case of IN and NOT IN clauses, if the list returned is empty, a guard variable is set to work around the inability for IN clauses to handle an empty list syntax. Subqueries can only be pulled out if they are not correlated. Correlated subqueries will be handled when we add the ability to execute expressions in VTGate. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

demmer

Overall this seems good to me.

I made a few comments that I think would improve clarity for both this change and other elements of the core builder interface, but none are fundamental blockers.

demmer · 2018-11-17T01:14:23Z

go/vt/vtgate/planbuilder/jointab.go

-	vars map[string]struct{}
+	refs     map[*column]string
+	vars     map[string]struct{}
+	varIndex int


It'd be great to have some comments here as to what these are used for.

Specifically it looks to me from reading the code that varIndex would be better named uniqueSuffix but even without the name it'd be good to have a comment.

Separate PR.

go/vt/vtgate/planbuilder/jointab.go

demmer · 2018-11-17T01:27:28Z

data/test/vtgate/filter_cases.txt

+# Note the improved Underlying plan as SelectIN.
+"select id from user where id in (select col from user)"
+{
+  "Original": "select id from user where id in (select col from user)",


One thing that occurred to me in reading this -- can we push down a subquery to a single shard if specified in the query?

Specifically suppose two tables are both sharded by a company_id column and we ran a query like:

select * from user where company_id=123 and id in (select user_id from admins where company_id=123);

In this case the query can be pushed down to whichever shard has both the user and user_admin.

Does the current implementation handle this efficiently?

go/vt/vtgate/engine/pullout_subquery.go

demmer · 2018-11-17T01:52:27Z

data/test/vtgate/filter_cases.txt

+}
+
+# cross-shard subquery in NOT IN clause.
+"select id from user where id not in (select col from user)"


I also think it would be incredibly useful for cases like this to include vtexplain test cases for each of these as well to be able to more easily see as a reviewer (and someone getting to know Vitess) which exact queries end up running for these various patterns.

go/vt/vtgate/engine/pullout_subquery.go

demmer · 2018-11-17T15:00:00Z

go/vt/vtgate/planbuilder/expr.go

 // If an expression has no references to the current query, then the left-most
 // origin is chosen as the default.
-func (pb *primitiveBuilder) findOrigin(expr sqlparser.Expr) (origin builder, pushExpr sqlparser.Expr, err error) {
+func (pb *primitiveBuilder) findOrigin(expr sqlparser.Expr) (pullouts []*pulloutSubquery, origin builder, pushExpr sqlparser.Expr, err error) {


The block comment above should probably be updated to indicate that this doesn't always bail if subqueries that can't be merged with the route (now) and also that the expressions might be rewritten, some subqueries are pulled out, etc.

Moreover, I think that similar to the comment on the Primitive() that findOrigin is a misleading name for this function, since it does more than finding the origin. Maybe something like analyzeExpr or buildExpr?

Separate PR.

demmer · 2018-11-17T20:03:54Z

go/vt/vtgate/planbuilder/pullout_subquery.go

+// Primitive satisfies the builder interface.
+func (ps *pulloutSubquery) Primitive() engine.Primitive {
+	ps.eSubquery.Subquery = ps.subquery.Primitive()
+	ps.eSubquery.Underlying = ps.underlying.Primitive()


Not new to this PR but it's non-intuitive that Primitive() actually mutates the builder data structures and doesn't just return the primitive itself, especially with the interface documentation of:

// Primitve returns the underlying primitive. Primitive() engine.Primitive

IMO this should be renamed to BuildPrimitive().

(Also I just noticed the Primitve typo in the doc comment)

Separate PR.

demmer · 2018-11-17T20:19:52Z

go/vt/vtgate/planbuilder/from.go

 }

 func (pb *primitiveBuilder) join(rpb *primitiveBuilder, ajoin *sqlparser.JoinTableExpr) error {
-	if ajoin != nil && ajoin.Condition.Using != nil {


I assume this join case now works because of subqueries? That's not immediately obvious to me.

This code was unreachable. The check was alread performed in the code path for newJoin in join.go. OCD cleanup...

demmer · 2018-11-17T20:19:59Z

go/vt/vtgate/planbuilder/from.go

 	if leftIsRoute && rightIsRoute {
 		// If both are routes, they have an opportunity
 		// to merge into one.
-		if lRoute.ERoute.Opcode == engine.SelectNext || rRoute.ERoute.Opcode == engine.SelectNext {


Same here. See new comments added below.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou

Deferred for separate PR:

Primtive -> Primitive in other places
Find better name for findOrigin
jointab comments
Primitive -> BuildPrimitive

Also, answering a comment I couldn't find in the PR: Subqueries that have the same route as the outer query will not be broken out. They will be pushed down as single unit. The new subquery code does not change this behavior.

sougou · 2018-11-17T21:53:54Z

go/vt/vtgate/planbuilder/from.go

 }

 func (pb *primitiveBuilder) join(rpb *primitiveBuilder, ajoin *sqlparser.JoinTableExpr) error {
-	if ajoin != nil && ajoin.Condition.Using != nil {


This code was unreachable. The check was alread performed in the code path for newJoin in join.go. OCD cleanup...

sougou · 2018-11-17T21:54:11Z

go/vt/vtgate/planbuilder/from.go

 	if leftIsRoute && rightIsRoute {
 		// If both are routes, they have an opportunity
 		// to merge into one.
-		if lRoute.ERoute.Opcode == engine.SelectNext || rRoute.ERoute.Opcode == engine.SelectNext {


Same here. See new comments added below.

go/vt/vtgate/engine/pullout_subquery.go

sougou · 2018-11-17T21:59:28Z

go/vt/vtgate/planbuilder/expr.go

 // If an expression has no references to the current query, then the left-most
 // origin is chosen as the default.
-func (pb *primitiveBuilder) findOrigin(expr sqlparser.Expr) (origin builder, pushExpr sqlparser.Expr, err error) {
+func (pb *primitiveBuilder) findOrigin(expr sqlparser.Expr) (pullouts []*pulloutSubquery, origin builder, pushExpr sqlparser.Expr, err error) {


Separate PR.

sougou · 2018-11-17T22:00:02Z

go/vt/vtgate/planbuilder/jointab.go

-	vars map[string]struct{}
+	refs     map[*column]string
+	vars     map[string]struct{}
+	varIndex int


Separate PR.

sougou · 2018-11-17T22:00:44Z

go/vt/vtgate/planbuilder/pullout_subquery.go

+// Primitive satisfies the builder interface.
+func (ps *pulloutSubquery) Primitive() engine.Primitive {
+	ps.eSubquery.Subquery = ps.subquery.Primitive()
+	ps.eSubquery.Underlying = ps.underlying.Primitive()


Separate PR.

demmer

I found one more spelling typo and had another naming suggestion. but I'm fine to move ahead with or without those changes.

data/test/vtexplain/multi-output/selectsharded-output.txt

demmer · 2018-11-18T02:52:38Z

go/vt/vtgate/planbuilder/jointab.go

+		jt.varIndex++
+		suffix := strconv.Itoa(jt.varIndex)
+		var1 := "__sq" + suffix
+		var2 := "__has_values" + suffix


Something which just occurred to me -- we should probably be more consistent in the naming scheme for these bind vars. Sorry for suggesting yet another renaming change, but I think it's worth it for clarity.

Specifically it seems like it'd be more consistent to change these to "__sq_valueX" and "__sq_has_valuesX" (or maybe invert the boolean sense of the latter and use "__sq_valueX" and "__sq_emptyX")?

I went with __sq_has_valuesX. The 'is_empty' approach felt less intuitive for the EXISTS case.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou requested review from demmer and rafael May 12, 2018 04:32

sougou added 3 commits October 30, 2018 21:03

vtgate planbuilder: name generation for subqueries

07650bc

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

subqueries: rebase master

9c64a05

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou mentioned this pull request Nov 15, 2018

select *: expand column list for tables with authoritative column lists #4336

Merged

demmer reviewed Nov 17, 2018

View reviewed changes

subquery: address review comments

16af2b8

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou commented Nov 17, 2018

View reviewed changes

sougou mentioned this pull request Nov 18, 2018

low performance on UNION src-d/gitbase#623

Closed

demmer approved these changes Nov 18, 2018

View reviewed changes

subquery: final touches

102645e

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou merged commit 6ec82cf into vitessio:master Nov 18, 2018

sougou deleted the sq2 branch November 18, 2018 17:48

rafael mentioned this pull request Dec 3, 2018

Slack vitess 2018 12 3.r0 tinyspeck/vitess#118

Merged

snyk-bot mentioned this pull request Jul 14, 2021

[Snyk] Security upgrade protractor from 3.3.0 to 5.0.0 SNiTEBoBy/vitess-gh#11

Open

Conversation

sougou commented May 12, 2018

Uh oh!

derekperkins commented Oct 15, 2018

Uh oh!

sougou commented Oct 15, 2018

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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

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!

demmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants