Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
26 changes: 26 additions & 0 deletions go/vt/sqlparser/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -2379,6 +2379,32 @@ func (node *ComparisonExpr) replace(from, to Expr) bool {
return replaceExprs(from, to, &node.Left, &node.Right, &node.Escape)
}

// IsImpossible returns true if the comparison in the expression can never evaluate to true.
// Note that this is not currently exhaustive to ALL impossible comparisons.
func (node *ComparisonExpr) IsImpossible() bool {
var left, right *SQLVal
var ok bool
if left, ok = node.Left.(*SQLVal); !ok {
return false
}
if right, ok = node.Right.(*SQLVal); !ok {
return false
}
if node.Operator == NotEqualStr && left.Type == right.Type {
if len(left.Val) != len(right.Val) {
return false
}

for i := range left.Val {
if left.Val[i] != right.Val[i] {
return false
}
}
return true
}
return false
}

// RangeCond represents a BETWEEN or a NOT BETWEEN expression.
type RangeCond struct {
Operator string
Expand Down
29 changes: 29 additions & 0 deletions go/vt/sqlparser/ast_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,35 @@ func TestIsAggregate(t *testing.T) {
}
}

func TestIsImpossible(t *testing.T) {
f := ComparisonExpr{
Operator: NotEqualStr,
Left: newIntVal("1"),
Right: newIntVal("1"),
}
if !f.IsImpossible() {
t.Error("IsImpossible: false, want true")
}

f = ComparisonExpr{
Operator: EqualStr,
Left: newIntVal("1"),
Right: newIntVal("1"),
}
if f.IsImpossible() {
t.Error("IsImpossible: true, want false")
}

f = ComparisonExpr{
Operator: NotEqualStr,
Left: newIntVal("1"),
Right: newIntVal("2"),
}
if f.IsImpossible() {
t.Error("IsImpossible: true, want false")
}
}

func TestReplaceExpr(t *testing.T) {
tcases := []struct {
in, out string
Expand Down
32 changes: 32 additions & 0 deletions go/vt/vttablet/endtoend/queries_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1789,6 +1789,38 @@ func TestQueries(t *testing.T) {
},
},
},
&framework.MultiCase{
Name: "impossible queries",
Cases: []framework.Testable{
&framework.TestCase{
Name: "specific column",
Query: "select eid from vitess_a where 1 != 1",
Rewritten: []string{
"select eid from vitess_a where 1 != 1",
},
RowsAffected: 0,
},
&framework.TestCase{
Name: "all columns",
Query: "select * from vitess_a where 1 != 1",
Rewritten: []string{
"select * from vitess_a where 1 != 1",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Backing out the fix in this PR makes another query show up here, which is nice to see.

},
RowsAffected: 0,
},
&framework.TestCase{
Name: "bind vars",
Query: "select :bv from vitess_a where 1 != 1",
BindVars: map[string]*querypb.BindVariable{
"bv": sqltypes.Int64BindVariable(1),
},
Rewritten: []string{
"select 1 from vitess_a where 1 != 1 limit 10001",
},
RowsAffected: 0,
},
},
},
}
for _, tcase := range testCases {
if err := tcase.Test("", client); err != nil {
Expand Down
8 changes: 8 additions & 0 deletions go/vt/vttablet/tabletserver/planbuilder/dml.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,14 @@ func analyzeSelect(sel *sqlparser.Select, tables map[string]*schema.Table) (plan
return nil, err
}

if sel.Where != nil {
comp, ok := sel.Where.Expr.(*sqlparser.ComparisonExpr)
if ok && comp.IsImpossible() {
plan.PlanID = PlanSelectImpossible
return plan, nil
}
}

// Check if it's a NEXT VALUE statement.
if nextVal, ok := sel.SelectExprs[0].(sqlparser.Nextval); ok {
if table.Type != schema.Sequence {
Expand Down
38 changes: 21 additions & 17 deletions go/vt/vttablet/tabletserver/planbuilder/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ const (
PlanOtherAdmin
// PlanMessageStream is used for streaming messages.
PlanMessageStream
// PlanSelectImpossible is used for where or having clauses that can never be true.
PlanSelectImpossible
// NumPlans stores the total number of plans
NumPlans
)
Expand All @@ -105,6 +107,7 @@ var planName = [NumPlans]string{
"OTHER_READ",
"OTHER_ADMIN",
"MESSAGE_STREAM",
"SELECT_IMPOSSIBLE",
}

func (pt PlanType) String() string {
Expand All @@ -126,7 +129,7 @@ func PlanByName(s string) (pt PlanType, ok bool) {

// IsSelect returns true if PlanType is about a select query.
func (pt PlanType) IsSelect() bool {
return pt == PlanPassSelect || pt == PlanSelectLock
return pt == PlanPassSelect || pt == PlanSelectLock || pt == PlanSelectImpossible
}

// MarshalJSON returns a json string for PlanType.
Expand All @@ -140,22 +143,23 @@ func (pt PlanType) MinRole() tableacl.Role {
}

var tableACLRoles = map[PlanType]tableacl.Role{
PlanPassSelect: tableacl.READER,
PlanSelectLock: tableacl.READER,
PlanSet: tableacl.READER,
PlanPassDML: tableacl.WRITER,
PlanDMLPK: tableacl.WRITER,
PlanDMLSubquery: tableacl.WRITER,
PlanInsertPK: tableacl.WRITER,
PlanInsertSubquery: tableacl.WRITER,
PlanInsertMessage: tableacl.WRITER,
PlanDDL: tableacl.ADMIN,
PlanSelectStream: tableacl.READER,
PlanOtherRead: tableacl.READER,
PlanOtherAdmin: tableacl.ADMIN,
PlanUpsertPK: tableacl.WRITER,
PlanNextval: tableacl.WRITER,
PlanMessageStream: tableacl.WRITER,
PlanPassSelect: tableacl.READER,
PlanSelectLock: tableacl.READER,
PlanSet: tableacl.READER,
PlanPassDML: tableacl.WRITER,
PlanDMLPK: tableacl.WRITER,
PlanDMLSubquery: tableacl.WRITER,
PlanInsertPK: tableacl.WRITER,
PlanInsertSubquery: tableacl.WRITER,
PlanInsertMessage: tableacl.WRITER,
PlanDDL: tableacl.ADMIN,
PlanSelectStream: tableacl.READER,
PlanOtherRead: tableacl.READER,
PlanOtherAdmin: tableacl.ADMIN,
PlanUpsertPK: tableacl.WRITER,
PlanNextval: tableacl.WRITER,
PlanMessageStream: tableacl.WRITER,
PlanSelectImpossible: tableacl.READER,
}

//_______________________________________________
Expand Down
15 changes: 13 additions & 2 deletions go/vt/vttablet/tabletserver/query_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,17 @@ func (qre *QueryExecutor) Execute() (reply *sqltypes.Result, err error) {
return qre.execDDL()
case planbuilder.PlanNextval:
return qre.execNextval()
case planbuilder.PlanSelectImpossible:
if qre.plan.Fields != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's possible for Fields to be nil for cases where there are bind vars in the select expressions. Code path is here: https://github.com/vitessio/vitess/blob/master/go/vt/vttablet/tabletserver/planbuilder/query_gen.go#L36. In that case we should still send the impossible query to mysql and return those results.

Strangely, I only see an endtoend test for this here: https://github.com/vitessio/vitess/blob/master/go/vt/vttablet/endtoend/queries_test.go#L454. There should ideally be a unit test for this in query_executor_test.go.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, I thought I handled that by treating PlanSelectImpossible as a normal select downstream, but will look into this.

return &sqltypes.Result{
Fields: qre.plan.Fields,
RowsAffected: 0,
InsertID: 0,
Rows: nil,
Extras: nil,
}, nil
}
break
}

if qre.transactionID != 0 {
Expand Down Expand Up @@ -137,7 +148,7 @@ func (qre *QueryExecutor) Execute() (reply *sqltypes.Result, err error) {
return qre.execUpsertPK(conn)
case planbuilder.PlanSet:
return qre.txFetch(conn, qre.plan.FullQuery, qre.bindVars, nil, nil, true, true)
case planbuilder.PlanPassSelect, planbuilder.PlanSelectLock:
case planbuilder.PlanPassSelect, planbuilder.PlanSelectLock, planbuilder.PlanSelectImpossible:
return qre.execDirect(conn)
default:
// handled above:
Expand All @@ -151,7 +162,7 @@ func (qre *QueryExecutor) Execute() (reply *sqltypes.Result, err error) {
}
} else {
switch qre.plan.PlanID {
case planbuilder.PlanPassSelect:
case planbuilder.PlanPassSelect, planbuilder.PlanSelectImpossible:
return qre.execSelect()
case planbuilder.PlanSelectLock:
return nil, vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "%s disallowed outside transaction", qre.plan.PlanID.String())
Expand Down
27 changes: 26 additions & 1 deletion go/vt/vttablet/tabletserver/query_executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1242,6 +1242,31 @@ func TestQueryExecutorPlanPassSelect(t *testing.T) {
}
}

func TestQueryExecutorPlanSelectImpossible(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As mentioned above, we'll need a test for the select :bv from t where 1 != 1 case.

db := setUpQueryExecutorTest(t)
defer db.Close()
query := "select * from test_table where 1 != 1"
want := &sqltypes.Result{
Fields: getTestTableFields(),
}
db.AddQuery(query, want)
db.AddQuery("select * from test_table where 1 != 1", &sqltypes.Result{
Fields: getTestTableFields(),
})
ctx := context.Background()
tsv := newTestTabletServer(ctx, noFlags, db)
qre := newTestQueryExecutor(ctx, tsv, query, 0)
defer tsv.StopService()
checkPlanID(t, planbuilder.PlanSelectImpossible, qre.plan.PlanID)
got, err := qre.Execute()
if err != nil {
t.Fatalf("qre.Execute() = %v, want nil", err)
}
if !reflect.DeepEqual(got, want) {
t.Fatalf("got: %v, want: %v", got, want)
}
}

func TestQueryExecutorPlanPassSelectSqlSelectLimit(t *testing.T) {
db := setUpQueryExecutorTest(t)
defer db.Close()
Expand Down Expand Up @@ -1732,7 +1757,7 @@ func TestQueryExecutorTableAclDualTableExempt(t *testing.T) {
query := "select * from test_table where 1 != 1"
qre := newTestQueryExecutor(ctx, tsv, query, 0)
defer tsv.StopService()
checkPlanID(t, planbuilder.PlanPassSelect, qre.plan.PlanID)
checkPlanID(t, planbuilder.PlanSelectImpossible, qre.plan.PlanID)
// query should fail because nobody has read access to test_table
_, err := qre.Execute()
if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED {
Expand Down