Range queries in from clause #3771

rafael · 2018-03-22T21:04:57Z

Description

Continues the work for Enhanced shard targeting #3666.
Adds the foundation for enhanced targeting inside from clauses.

Summary of changes

Adds all the ground work to support range queries in from clauses. At the moment, only added support for DELETE statements.
Big refactor of engine.go to make destination a first class citizen, instead of a shard target. I think this simplifies the code even more and expands on using destination and the refactor that started in: Enhanced shard targeting part I #3692.
Renames DeleteSharded to DeleteScatter, that's more consistent with existent opcodes.
Refactor dml.go planbuilder into insert.go and delete.go. Also, to be more consistent with all the other primitives.

* Some general refactor of plan builders (move them to have their own files). * Refactor parse destination code and put it in key package. * Add general support for range queries in delete statements. Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

rafael · 2018-03-22T21:15:43Z

go/vt/vtgate/executor.go

@sougou - this looks like the right thing here, but wanted to double check with you.

This is ok for now. Ideally, we want shard targeting to be honored here also. But can be changed later.

Signed-off-by: Rafael Chacon <rafael@slack-corp.com> Delete

rafael · 2018-03-22T21:21:31Z

go/vt/vtgate/planbuilder/delete.go

+		return nil, err
+	}
+	edel.Table = table
+	if destTarget.Destination != nil {


This the only real change here.

sougou

This is my initial pass.
Got some comments.

I think I like this approach. Need to let it sink in.

sougou · 2018-03-23T22:36:44Z

go/vt/key/key.go

 	return buf
 }

+type DestinationTarget struct {


This name is confusing. How about QualifiedDestination?

I'm actually wondering if we even need this type. Could we get away with always using the relevant individual fields, without the structure? One reason I'm saying that is for instance the TabletType is not useful in delete.go below, as a delete would always be on the master.

The parse method can look like:
parse(toParse string, defaultType tabletType) (keyspace string, tabletType, Destination, error)

@alainjobart - I originally went down this approach but thought it was too verbose when we actually need the all the values:

keyspace, tabletType, dest, err := key.ParseDestination(targetString, defaultTabletType)

I leaned towards the struct because it was more concise.

Another reason for this is we pass keysapce/tablettype + single Destination, or keyspace/tablettype + []Destination in a few methods. Like in srvtoporesolver.go:

func (r *Resolver) ResolveDestinations(ctx context.Context, keyspace string, tabletType topodatapb.TabletType, ids []*querypb.Value, destinations []key.Destination) ([]*ResolvedShard, [][]*querypb.Value, error) {

I agree it's more concise with the structure, but then we'd need another one with []Destination too, and it's starting to bloat the code...

(note I'm not opposed to thi that much, it just feels overkill)

I think it's better to not have the type (for now). If the combination of vars gains widespread usage, we should look at making it a core type used everywhere.

Not having the type also avoids the argument about what to name it :).

These are good points!

I updated the code to not have this data structure.

sougou · 2018-03-23T22:42:54Z

go/vt/vtgate/executor.go

This is ok for now. Ideally, we want shard targeting to be honored here also. But can be changed later.

sougou · 2018-03-23T22:48:27Z

go/vt/vtgate/planbuilder/dml.go

 	"vitess.io/vitess/go/sqltypes"
 	"vitess.io/vitess/go/vt/sqlparser"
-	"vitess.io/vitess/go/vt/vterrors"
-	"vitess.io/vitess/go/vt/vtgate/engine"


This file should be deleted, and its functions moved elsewhere right?

sougou · 2018-03-23T22:52:03Z

go/vt/vtgate/planbuilder/builder.go

 type VSchema interface {
-	FindTable(tablename sqlparser.TableName) (*vindexes.Table, error)
-	FindTableOrVindex(tablename sqlparser.TableName) (*vindexes.Table, vindexes.Vindex, error)
+	FindTable(tablename sqlparser.TableName) (*vindexes.Table, key.DestinationTarget, error)


Let's rename this to VCursor, because the FindTable function signature doesn't match the one in VSchema any more.

alainjobart · 2018-03-26T18:28:57Z

go/vt/key/key.go

 	return buf
 }

+type DestinationTarget struct {


I'm actually wondering if we even need this type. Could we get away with always using the relevant individual fields, without the structure? One reason I'm saying that is for instance the TabletType is not useful in delete.go below, as a delete would always be on the master.

The parse method can look like:
parse(toParse string, defaultType tabletType) (keyspace string, tabletType, Destination, error)

alainjobart · 2018-03-26T18:30:34Z

go/vt/vtgate/executor.go

-func (e *Executor) destinationExec(ctx context.Context, safeSession *SafeSession, sql string, bindVars map[string]*querypb.BindVariable, target querypb.Target, destination key.Destination, logStats *LogStats) (*sqltypes.Result, error) {
-	return e.resolver.Execute(ctx, sql, bindVars, target.Keyspace, target.TabletType, destination, safeSession.Session, false /* notInTransaction */, safeSession.Options, logStats)
+func (e *Executor) destinationExec(ctx context.Context, safeSession *SafeSession, sql string, bindVars map[string]*querypb.BindVariable, destTarget key.DestinationTarget, logStats *LogStats) (*sqltypes.Result, error) {
+	return e.resolver.Execute(ctx, sql, bindVars, destTarget.Keyspace, destTarget.TabletType, destTarget.Destination, safeSession.Session, false /* notInTransaction */, safeSession.Options, logStats)


I feel that the previous one was wrong, it is:
target querypb.Target, destination key.Destination
should have been:
keyspace string, targetType, destination
See my previous comment, we don't really need a structure for this, do we?

sougou · 2018-03-27T16:06:32Z

go/vt/key/key.go

 	return buf
 }

+type DestinationTarget struct {


I think it's better to not have the type (for now). If the combination of vars gains widespread usage, we should look at making it a core type used everywhere.

Not having the type also avoids the argument about what to name it :).

sougou · 2018-03-27T16:10:42Z

go/vt/key/key.go

+
+// ParseDestination parses the string representation of a Destionation
+// of the form keyspace:shard@tablet_type. You can use a / instead of a :.
+func ParseDestination(targetString string, defaultTabletType topodatapb.TabletType) (DestinationTarget, error) {


I feel like this function belongs inside executor's parseDestinationTarget, and we should have vcursor call that instead. It will keep this file simple. Open to counter-arguments.

@sougou - I think this is in the only comment I haven't addressed. I will think a bit more about it.

I thought about this. I think it makes sense. I wanted to keep the one here as a more generic version, but now I'm thinking that the slight difference in implementation could bite us in the future.

What do you think if I move this method to the executor and make it private. The reason for this is that I would like to have something hook into it easily in the context of the tests in plan_test.go that does not require a real executor.

Then in vcursor use ParseDestinationTarget.

Thoughts?

Sounds good.

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

* Refactor key.topo to not have an extra data structure. This could be added in the future if we need it. For now keep it simple and avoid the overhead of adding it. Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

alainjobart

Looks good now, mostly cosmetic comments :)

alainjobart · 2018-03-28T15:18:31Z

go/vt/key/key.go

+func parseTabletType(param string) topodatapb.TabletType {
+	value, ok := topodatapb.TabletType_value[strings.ToUpper(param)]
+	if !ok {
+		return topodatapb.TabletType_UNKNOWN


I think this method should return an error:
func parseTabletType(param string) (topodatapb.TabletType, error) {

After I moved ParseDestination out of key.go, I don't longer need this method and I'm using topoproto.ParseTabletType

alainjobart · 2018-03-28T15:18:57Z

go/vt/key/key_test.go

+		targetString string
+		dest         Destination
+		keyspace     string
+		tabletType   topodatapb.TabletType


adding 'errorString string' and testing for error cases might be good here too.

alainjobart · 2018-03-28T15:19:13Z

go/vt/vtgate/engine/delete.go

 	// Keyspace specifies the keyspace to send the query to.
 	Keyspace *vindexes.Keyspace

+	// Keyspace specifies the keyspace to send the query to.


Update comment.

alainjobart · 2018-03-28T15:20:25Z

go/vt/vtgate/executor.go

-	}
-
-	if keyRange != nil || target.Shard != "" {
+func (e *Executor) handleExec(ctx context.Context, safeSession *SafeSession, sql string, bindVars map[string]*querypb.BindVariable, dest key.Destination, destKeyspace string, destTabletType topodatapb.TabletType, logStats *LogStats) (*sqltypes.Result, error) {


Th usual ordering of the parameters is keyspace, tablet type, and then destination.

alainjobart · 2018-03-28T15:22:23Z

go/vt/vtgate/planbuilder/builder.go

-	FindTableOrVindex(tablename sqlparser.TableName) (*vindexes.Table, vindexes.Vindex, error)
+type ContextVSchema interface {
+	FindTable(tablename sqlparser.TableName) (*vindexes.Table, key.Destination, string, topodatapb.TabletType, error)
+	FindTableOrVindex(tablename sqlparser.TableName) (*vindexes.Table, vindexes.Vindex, key.Destination, string, topodatapb.TabletType, error)


same for these two here, keyspace, tablettype,destination is the usual order.

rafael · 2018-04-02T23:43:35Z

@sougou , @alainjobart I think all comments have been addressed! This should be ready.

* Make sure vcursor implementation uses same ParseDestination logic as executor. * Address other PR comments and refactor methods to better respect conventions within Vitess codebase. Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

Rafael Chacon added 3 commits March 22, 2018 13:10

Refactor engine to make destination a first class citizen

6fe34a5

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

Add more validations and name refactor

0841b58

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

rafael force-pushed the range-queries-in-from-clause branch from f5769f2 to 775a2d8 Compare March 22, 2018 21:09

rafael commented Mar 22, 2018

View reviewed changes

More consistent naming

7ef8184

Signed-off-by: Rafael Chacon <rafael@slack-corp.com> Delete

rafael force-pushed the range-queries-in-from-clause branch from 775a2d8 to 7ef8184 Compare March 22, 2018 21:19

rafael commented Mar 22, 2018

View reviewed changes

rafael requested review from alainjobart and sougou March 23, 2018 00:02

sougou reviewed Mar 23, 2018

View reviewed changes

alainjobart reviewed Mar 26, 2018

View reviewed changes

sougou reviewed Mar 27, 2018

View reviewed changes

Rafael Chacon added 2 commits March 27, 2018 17:06

Address PR comments, add more tests

e92d03a

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

Do not add a new data structure for a qualified destination

c29eb8e

* Refactor key.topo to not have an extra data structure. This could be added in the future if we need it. For now keep it simple and avoid the overhead of adding it. Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

rafael force-pushed the range-queries-in-from-clause branch from ae1c1eb to c29eb8e Compare March 28, 2018 00:06

Rename builder VSchema interface to better reflect current behavior

416e2e7

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

rafael force-pushed the range-queries-in-from-clause branch from e60cb70 to 416e2e7 Compare March 28, 2018 00:11

Better error messages and adds more tests

4f9089e

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

alainjobart reviewed Mar 28, 2018

View reviewed changes

rafael force-pushed the range-queries-in-from-clause branch from 3121589 to 2010712 Compare April 2, 2018 23:41

Update per PR feedback

6445dd8

* Make sure vcursor implementation uses same ParseDestination logic as executor. * Address other PR comments and refactor methods to better respect conventions within Vitess codebase. Signed-off-by: Rafael Chacon <rafael@slack-corp.com>

rafael force-pushed the range-queries-in-from-clause branch from 2010712 to 6445dd8 Compare April 3, 2018 00:08

sougou approved these changes Apr 3, 2018

View reviewed changes

sougou merged commit fcb135c into vitessio:master Apr 3, 2018

rafael deleted the range-queries-in-from-clause branch April 3, 2018 19:09

Range queries in from clause #3771

Range queries in from clause #3771

Uh oh!

Conversation

rafael commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary of changes

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!

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!

rafael Mar 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!

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!

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

rafael commented Apr 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rafael commented Mar 22, 2018 •

edited

Loading

rafael Mar 28, 2018 •

edited

Loading