From 14991c96ea4da06574f988598027e1a6a3bafada Mon Sep 17 00:00:00 2001 From: Sugu Sougoumarane Date: Wed, 11 Dec 2019 20:48:49 -0800 Subject: [PATCH] routing: improve dual and left joins Fixes #4772 Fixes #5508 Previously, vitess was conservative about using vindexes for tables involving left joins. After some reasoning, we've determined that it's actually safe (and more efficient) to use them. In existing functionality, a reference table had to be on the RHS of the analysis. There was no benefit if it was on the LHS. When we changed dual to be a Reference tables, it caused a regression. If dual was on the LHS of a join or subquery, then it would get treated as cross-shard. The new change handles the case of ref tables to be on the LHS. This also fixes the regression. Signed-off-by: Sugu Sougoumarane --- go.mod | 3 -- go/vt/vtgate/planbuilder/route_option.go | 24 ++++++++------ .../planbuilder/testdata/filter_cases.txt | 6 +++- .../planbuilder/testdata/from_cases.txt | 32 +++++-------------- .../testdata/unsupported_cases.txt | 5 --- 5 files changed, 27 insertions(+), 43 deletions(-) diff --git a/go.mod b/go.mod index 44d8c461dd1..6308bc259ca 100644 --- a/go.mod +++ b/go.mod @@ -25,7 +25,6 @@ require ( github.com/golang/mock v1.3.1 github.com/golang/protobuf v1.3.2 github.com/golang/snappy v0.0.0-20170215233205-553a64147049 - github.com/google/btree v1.0.0 // indirect github.com/google/shlex v0.0.0-20181106134648-c34317bd91bf // indirect github.com/gorilla/websocket v0.0.0-20160912153041-2d1e4548da23 github.com/grpc-ecosystem/go-grpc-middleware v1.1.0 @@ -50,8 +49,6 @@ require ( github.com/minio/minio-go v0.0.0-20190131015406-c8a261de75c1 github.com/mitchellh/go-testing-interface v1.0.0 // indirect github.com/mitchellh/mapstructure v1.1.2 // indirect - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect - github.com/modern-go/reflect2 v1.0.1 // indirect github.com/olekukonko/tablewriter v0.0.0-20160115111002-cca8bbc07984 github.com/opentracing-contrib/go-grpc v0.0.0-20180928155321-4b5a12d3ff02 github.com/opentracing/opentracing-go v1.1.0 diff --git a/go/vt/vtgate/planbuilder/route_option.go b/go/vt/vtgate/planbuilder/route_option.go index 1d6ba1c3679..fa3bab2c6a2 100644 --- a/go/vt/vtgate/planbuilder/route_option.go +++ b/go/vt/vtgate/planbuilder/route_option.go @@ -87,12 +87,8 @@ func (ro *routeOption) JoinCanMerge(pb *primitiveBuilder, rro *routeOption, ajoi } func (ro *routeOption) MergeJoin(rro *routeOption, isLeftJoin bool) { + ro.merge(rro) ro.vschemaTable = nil - ro.substitutions = append(ro.substitutions, rro.substitutions...) - if isLeftJoin { - return - } - // Add RHS vindexes only if it's not a left join. for c, v := range rro.vindexMap { if ro.vindexMap == nil { ro.vindexMap = make(map[*column]vindexes.SingleColumn) @@ -101,6 +97,16 @@ func (ro *routeOption) MergeJoin(rro *routeOption, isLeftJoin bool) { } } +// merge merges two routeOptions. If the LHS (ro) is a SelectReference, +// then the RHS option values supersede LHS. +func (ro *routeOption) merge(rro *routeOption) { + if ro.eroute.Opcode == engine.SelectReference { + // Swap the values and then merge. + *ro, *rro = *rro, *ro + } + ro.substitutions = append(ro.substitutions, rro.substitutions...) +} + func (ro *routeOption) SubqueryCanMerge(pb *primitiveBuilder, inner *routeOption) bool { return ro.canMerge(inner, func() bool { switch vals := inner.condition.(type) { @@ -114,7 +120,7 @@ func (ro *routeOption) SubqueryCanMerge(pb *primitiveBuilder, inner *routeOption } func (ro *routeOption) MergeSubquery(subqueryOption *routeOption) { - ro.substitutions = append(ro.substitutions, subqueryOption.substitutions...) + ro.merge(subqueryOption) } func (ro *routeOption) UnionCanMerge(rro *routeOption) bool { @@ -122,8 +128,8 @@ func (ro *routeOption) UnionCanMerge(rro *routeOption) bool { } func (ro *routeOption) MergeUnion(rro *routeOption) { + ro.merge(rro) ro.vschemaTable = nil - ro.substitutions = append(ro.substitutions, rro.substitutions...) } func (ro *routeOption) SubqueryToTable(rb *route, vindexMap map[*column]vindexes.SingleColumn) { @@ -149,9 +155,7 @@ func (ro *routeOption) canMerge(rro *routeOption, customCheck func() bool) bool return true } case engine.SelectReference: - // TODO(sougou): this can be changed to true, but we'll have - // to merge against rro insteal of ro. - return false + return true case engine.SelectNext: return false } diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index 0130fe08b72..53fae69ba05 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -233,13 +233,17 @@ { "Original": "select user_extra.id from user left join user_extra on user.id = user_extra.user_id where user_extra.user_id = 5", "Instructions": { - "Opcode": "SelectScatter", + "Opcode": "SelectEqualUnique", "Keyspace": { "Name": "user", "Sharded": true }, "Query": "select user_extra.id from user left join user_extra on user.id = user_extra.user_id where user_extra.user_id = 5", "FieldQuery": "select user_extra.id from user left join user_extra on user.id = user_extra.user_id where 1 != 1", + "Vindex": "user_index", + "Values": [ + 5 + ], "Table": "user" } } diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.txt b/go/vt/vtgate/planbuilder/testdata/from_cases.txt index 6428be6a65f..67943ac9df6 100644 --- a/go/vt/vtgate/planbuilder/testdata/from_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/from_cases.txt @@ -927,35 +927,19 @@ } } -# reference table doesn't merge with other opcodes yet. +# reference table can merge with other opcodes left to right. "select ref.col from ref join user" { "Original": "select ref.col from ref join user", "Instructions": { - "Opcode": "Join", - "Left": { - "Opcode": "SelectReference", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "Query": "select ref.col from ref", - "FieldQuery": "select ref.col from ref where 1 != 1", - "Table": "ref" - }, - "Right": { - "Opcode": "SelectScatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "Query": "select 1 from user", - "FieldQuery": "select 1 from user where 1 != 1", - "Table": "user" + "Opcode": "SelectScatter", + "Keyspace": { + "Name": "user", + "Sharded": true }, - "Cols": [ - -1 - ] + "Query": "select ref.col from ref join user", + "FieldQuery": "select ref.col from ref join user where 1 != 1", + "Table": "user" } } diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 77cee7da742..67a05f8aeea 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -432,8 +432,3 @@ "select func(keyspace_id) from user_index where id = :id" "unsupported: expression on results of a vindex function" - -# Multi-table unique vindex constraint on left table of left join, two levels of join, simple aggregation -# This should work, but doesn't. See https://github.com/vitessio/vitess/issues/4772 -"select user.id, count(*) from user left join user_extra ue1 on user.id = ue1.user_id left join user_extra ue2 on ue1.user_id = ue2.user_id group by user.id" -"unsupported: cross-shard query with aggregates"