Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvements - Planner #16631

Merged
merged 6 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions go/vt/vtgate/planbuilder/operators/aggregator.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,6 @@ func (a *Aggregator) Inputs() []Operator {
}

func (a *Aggregator) SetInputs(operators []Operator) {
if len(operators) != 1 {
panic(fmt.Sprintf("unexpected number of operators as input in aggregator: %d", len(operators)))
}
a.Source = operators[0]
}

Expand Down
3 changes: 0 additions & 3 deletions go/vt/vtgate/planbuilder/operators/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ func (d *Delete) Inputs() []Operator {
}

func (d *Delete) SetInputs(inputs []Operator) {
if len(inputs) != 1 {
panic(vterrors.VT13001("unexpected number of inputs for Delete operator"))
}
d.Source = inputs[0]
}

Expand Down
3 changes: 0 additions & 3 deletions go/vt/vtgate/planbuilder/operators/dml_with_input.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ func (d *DMLWithInput) Inputs() []Operator {
}

func (d *DMLWithInput) SetInputs(inputs []Operator) {
if len(inputs) < 2 {
panic("unexpected number of inputs for DMLWithInput operator")
}
d.Source = inputs[0]
d.DML = inputs[1:]
}
Expand Down
3 changes: 0 additions & 3 deletions go/vt/vtgate/planbuilder/operators/fk_cascade.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ func (fkc *FkCascade) Inputs() []Operator {

// SetInputs implements the Operator interface
func (fkc *FkCascade) SetInputs(operators []Operator) {
if len(operators) < 2 {
panic("incorrect count of inputs for FkCascade")
}
Comment on lines -64 to -66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should put all these checks behind a debug mode? 🤔 Would that help us in debugging when a user reports a query is not working as intended? Also, won't removing these make some plans pass that would otherwise panic that could return wrong rows?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code so that we now only use SetInputs() on the output of GetInputs(). Is that enough you think?

fkc.Parent = operators[0]
fkc.Selection = operators[1]
for idx, operator := range operators {
Expand Down
3 changes: 0 additions & 3 deletions go/vt/vtgate/planbuilder/operators/fk_verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ func (fkv *FkVerify) Inputs() []Operator {
// SetInputs implements the Operator interface
func (fkv *FkVerify) SetInputs(operators []Operator) {
fkv.Input = operators[0]
if len(fkv.Verify) != len(operators)-1 {
panic("mismatched number of verify inputs")
}
for i := 1; i < len(operators); i++ {
fkv.Verify[i-1].Op = operators[i]
}
Expand Down
3 changes: 2 additions & 1 deletion go/vt/vtgate/planbuilder/operators/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ type (
// Inputs returns the inputs for this operator
Inputs() []Operator

// SetInputs changes the inputs for this op
// SetInputs changes the inputs for this op.
// We don't need to check the size of the inputs, as the planner will ensure that the inputs are correct
SetInputs([]Operator)

// AddPredicate is used to push predicates. It pushed it as far down as is possible in the tree.
Expand Down
11 changes: 4 additions & 7 deletions go/vt/vtgate/planbuilder/operators/rewriters.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func bottomUp(
}

if anythingChanged.Changed() {
root = root.Clone(newInputs)
root.SetInputs(newInputs)
}

newOp, treeIdentity := rewriter(root, rootID, isRoot)
Expand Down Expand Up @@ -257,11 +257,7 @@ func breakableTopDown(
}
}

if anythingChanged.Changed() {
return newOp, NoRewrite, nil
}

return newOp.Clone(newInputs), anythingChanged, nil
return newOp, anythingChanged, nil
}

// topDown is a helper function that recursively traverses the operator tree from the
Expand Down Expand Up @@ -301,7 +297,8 @@ func topDown(
}

if anythingChanged != NoRewrite {
return root.Clone(newInputs), anythingChanged
root.SetInputs(newInputs)
return root, anythingChanged
}

return root, NoRewrite
Expand Down
18 changes: 12 additions & 6 deletions go/vt/vtgate/planbuilder/operators/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@ package operators
import (
"fmt"

"vitess.io/vitess/go/mysql/collations"
"vitess.io/vitess/go/slice"
"vitess.io/vitess/go/vt/key"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
"vitess.io/vitess/go/vt/sqlparser"
"vitess.io/vitess/go/vt/vtenv"
"vitess.io/vitess/go/vt/vterrors"
"vitess.io/vitess/go/vt/vtgate/engine"
"vitess.io/vitess/go/vt/vtgate/evalengine"
Expand Down Expand Up @@ -121,7 +119,7 @@ func UpdateRoutingLogic(ctx *plancontext.PlanningContext, expr sqlparser.Expr, r
}
nr := &NoneRouting{keyspace: ks}

if isConstantFalse(ctx.VSchema.Environment(), expr, ctx.VSchema.ConnCollation()) {
if isConstantFalse(ctx, expr) {
return nr
}

Expand Down Expand Up @@ -165,11 +163,19 @@ func UpdateRoutingLogic(ctx *plancontext.PlanningContext, expr sqlparser.Expr, r

// isConstantFalse checks whether this predicate can be evaluated at plan-time. If it returns `false` or `null`,
// we know that the query will not return anything, and this can be used to produce better plans
func isConstantFalse(env *vtenv.Environment, expr sqlparser.Expr, collation collations.ID) bool {
func isConstantFalse(ctx *plancontext.PlanningContext, expr sqlparser.Expr) bool {
if !ctx.SemTable.RecursiveDeps(expr).IsEmpty() {
// we have column dependencies, so we can be pretty sure
// we won't be able to use the evalengine to check if this is constant false
return false
}
env := ctx.VSchema.Environment()
collation := ctx.VSchema.ConnCollation()
eenv := evalengine.EmptyExpressionEnv(env)
eexpr, err := evalengine.Translate(expr, &evalengine.Config{
Collation: collation,
Environment: env,
Collation: collation,
Environment: env,
NoCompilation: true,
})
if err != nil {
return false
Expand Down
3 changes: 0 additions & 3 deletions go/vt/vtgate/planbuilder/operators/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,6 @@ func (u *Update) Inputs() []Operator {
}

func (u *Update) SetInputs(inputs []Operator) {
if len(inputs) != 1 {
panic(vterrors.VT13001("unexpected number of inputs for Update operator"))
}
u.Source = inputs[0]
}

Expand Down
29 changes: 22 additions & 7 deletions go/vt/vtgate/semantics/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package semantics

import (
"fmt"

querypb "vitess.io/vitess/go/vt/proto/query"
"vitess.io/vitess/go/vt/sqlparser"
"vitess.io/vitess/go/vt/vtgate/evalengine"
Expand All @@ -29,6 +31,7 @@ type (
empty() bool
get(col *sqlparser.ColName) (dependency, error)
merge(other dependencies, allowMulti bool) dependencies
debugString() string
}
dependency struct {
certain bool
Expand Down Expand Up @@ -100,6 +103,10 @@ func (u *uncertain) merge(d dependencies, _ bool) dependencies {
}
}

func (u *uncertain) debugString() string {
return fmt.Sprintf("uncertain: %v %v %s", u.direct, u.recursive, u.typ.Type().String())
}

func (c *certain) empty() bool {
return false
}
Expand All @@ -117,26 +124,34 @@ func (c *certain) merge(d dependencies, allowMulti bool) dependencies {
if d.recursive == c.recursive {
return c
}
c.direct = c.direct.Merge(d.direct)
c.recursive = c.recursive.Merge(d.recursive)

res := createCertain(c.direct.Merge(d.direct), c.recursive.Merge(d.recursive), c.typ)
if !allowMulti {
c.err = true
res.err = true
}

return c
return res
}

return c
}

func (n *nothing) empty() bool {
func (c *certain) debugString() string {
return fmt.Sprintf("certain: %v %v %s", c.direct, c.recursive, c.typ.Type().String())
}

func (*nothing) empty() bool {
return true
}

func (n *nothing) get(*sqlparser.ColName) (dependency, error) {
func (*nothing) get(*sqlparser.ColName) (dependency, error) {
return dependency{certain: true}, nil
}

func (n *nothing) merge(d dependencies, _ bool) dependencies {
func (*nothing) merge(d dependencies, _ bool) dependencies {
return d
}

func (*nothing) debugString() string {
return "nothing"
}
25 changes: 19 additions & 6 deletions go/vt/vtgate/semantics/real_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,36 @@ type RealTable struct {
VindexHint *sqlparser.IndexHint
isInfSchema bool
collationEnv *collations.Environment
cache map[string]dependencies
}

var _ TableInfo = (*RealTable)(nil)

// dependencies implements the TableInfo interface
func (r *RealTable) dependencies(colName string, org originable) (dependencies, error) {
ts := org.tableSetFor(r.ASTNode)
for _, info := range r.getColumns(false /* ignoreInvisbleCol */) {
if strings.EqualFold(info.Name, colName) {
return createCertain(ts, ts, info.Type), nil
func (r *RealTable) dependencies(colName string, org originable) (deps dependencies, err error) {
var myID *TableSet
if r.cache == nil {
r.cache = make(map[string]dependencies)
ts := org.tableSetFor(r.ASTNode)
myID = &ts
for _, info := range r.getColumns(false /* ignoreInvisbleCol */) {
r.cache[strings.ToLower(info.Name)] = createCertain(ts, ts, info.Type)
}
}

if deps, ok := r.cache[strings.ToLower(colName)]; ok {
return deps, nil
}

if r.authoritative() {
return &nothing{}, nil
}
return createUncertain(ts, ts), nil

if myID == nil {
ts := org.tableSetFor(r.ASTNode)
myID = &ts
}
return createUncertain(*myID, *myID), nil
}

// GetTables implements the TableInfo interface
Expand Down
Loading