Add a special plan type for impossible queries #4510
Conversation
| @@ -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 { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hm, I thought I handled that by treating PlanSelectImpossible as a normal select downstream, but will look into this.
go/vt/sqlparser/ast.go
Outdated
| // 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 { | ||
| return node.Operator == NotEqualStr && reflect.DeepEqual(node.Left, node.Right) |
There was a problem hiding this comment.
We've had a policy that DeepEqual shouldn't be used in the query serving path. This is because its cost is unpredictable. However, I'll be open to arguments about why it makes sense in this particular case. Let me know what you think.
There was a problem hiding this comment.
I was just trying to be a good oss citizen by adding support for any impossible query users might throw out, in this case I can just unroll the check to something more robust.
| @@ -1242,6 +1242,31 @@ func TestQueryExecutorPlanPassSelect(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestQueryExecutorPlanSelectImpossible(t *testing.T) { | |||
There was a problem hiding this comment.
As mentioned above, we'll need a test for the select :bv from t where 1 != 1 case.
eeSeeGee
force-pushed
the
young.20190209.impossible_plan
branch
2 times, most recently
from
af27948
to
9c8a4b7
Compare
| Name: "all columns", | ||
| Query: "select * from vitess_a where 1 != 1", | ||
| Rewritten: []string{ | ||
| "select * from vitess_a where 1 != 1", |
There was a problem hiding this comment.
Backing out the fix in this PR makes another query show up here, which is nice to see.
|
The tests don't look happy. |
Hm, not sure what's going on there. |
Signed-off-by: Aaron Young <young@squareup.com>
eeSeeGee
force-pushed
the
young.20190209.impossible_plan
branch
from
9c8a4b7
to
023f62e
Compare
This fixes an annoying case where a missed lookup will spam a single shard with the same impossible query over and over, because the gate doesn't cache columns.
Signed-off-by: Aaron Young young@squareup.com