Skip to content

Commit

Permalink
Improve replication plan error messages
Browse files Browse the repository at this point in the history 
Signed-off-by: Matt Lord <[email protected]>
  • Loading branch information
@mattlord
mattlord committed Dec 11, 2023
1 parent 48a60d8 commit 249a3f8
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 40 deletions.
47 changes: 21 additions & 26 deletions go/vt/vttablet/tabletmanager/vreplication/replicator_plan_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@ import (
"strings"
"testing"

"vitess.io/vitess/go/vt/binlog/binlogplayer"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/vt/binlog/binlogplayer"

binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata"
)

Expand Down Expand Up @@ -571,7 +572,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "bad query",
}},
},
err: "syntax error at position 4 near 'bad'",
err: `failed to parse query "bad query": syntax error at position 4 near 'bad'`,
}, {
// not a select
input: &binlogdatapb.Filter{
Expand All @@ -580,7 +581,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "update t1 set val=1",
}},
},
err: "unexpected: update t1 set val = 1",
err: "unsupported non-select statement in query: update t1 set val = 1",
}, {
// no distinct
input: &binlogdatapb.Filter{
Expand All @@ -589,7 +590,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select distinct c1 from t1",
}},
},
err: "unexpected: select distinct c1 from t1",
err: "unsupported distinct clause in query: select distinct c1 from t1",
}, {
// no ',' join
input: &binlogdatapb.Filter{
Expand All @@ -598,7 +599,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select * from t1, t2",
}},
},
err: "unexpected: select * from t1, t2",
err: "unsupported multi-table query: select * from t1, t2",
}, {
// no join
input: &binlogdatapb.Filter{
Expand All @@ -607,7 +608,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select * from t1 join t2",
}},
},
err: "unexpected: select * from t1 join t2",
err: "unsupported expression (*sqlparser.JoinTableExpr) in from clause: select * from t1 join t2",
}, {
// no subqueries
input: &binlogdatapb.Filter{
Expand All @@ -616,7 +617,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select * from (select * from t2) as a",
}},
},
err: "unexpected: select * from (select * from t2) as a",
err: "unsupported from source (*sqlparser.DerivedTable) in query: select * from (select * from t2) as a",
}, {
// cannot combine '*' with other
input: &binlogdatapb.Filter{
Expand All @@ -625,7 +626,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select *, c1 from t1",
}},
},
err: "unexpected: select *, c1 from t1",
err: "unsupported mix of '*' and columns in query: select *, c1 from t1",
}, {
// cannot combine '*' with other (different code path)
input: &binlogdatapb.Filter{
Expand All @@ -634,7 +635,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select c1, * from t1",
}},
},
err: "unexpected: *",
err: "invalid expression: *",
}, {
// no distinct in func
input: &binlogdatapb.Filter{
Expand All @@ -643,7 +644,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select hour(distinct c1) as a from t1",
}},
},
err: "syntax error at position 21 near 'distinct'",
err: `failed to parse query "select hour(distinct c1) as a from t1": syntax error at position 21 near 'distinct'`,
}, {
// funcs need alias
input: &binlogdatapb.Filter{
Expand All @@ -670,7 +671,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select sum(*) as c from t1",
}},
},
err: "syntax error at position 13",
err: `failed to parse query "select sum(*) as c from t1": syntax error at position 13`,
}, {
// sum should have only one argument
input: &binlogdatapb.Filter{
Expand All @@ -679,7 +680,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select sum(a, b) as c from t1",
}},
},
err: "syntax error at position 14",
err: `failed to parse query "select sum(a, b) as c from t1": syntax error at position 14`,
}, {
// no complex expr in sum
input: &binlogdatapb.Filter{
Expand All @@ -688,7 +689,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select sum(a + b) as c from t1",
}},
},
err: "unexpected: sum(a + b)",
err: "unsupported non-column name in sum clause: sum(a + b)",
}, {
// no complex expr in group by
input: &binlogdatapb.Filter{
Expand All @@ -697,7 +698,7 @@ func TestBuildPlayerPlan(t *testing.T) {
Filter: "select a from t1 group by a + 1",
}},
},
err: "unexpected: a + 1",
err: "unsupported non-column name or alias in group by clause: a + 1",
}, {
// group by does not reference alias
input: &binlogdatapb.Filter{
Expand Down Expand Up @@ -734,28 +735,22 @@ func TestBuildPlayerPlan(t *testing.T) {

for _, tcase := range testcases {
plan, err := buildReplicatorPlan(getSource(tcase.input), PrimaryKeyInfos, nil, binlogplayer.NewStats())
gotPlan, _ := json.Marshal(plan)
wantPlan, _ := json.Marshal(tcase.plan)
if string(gotPlan) != string(wantPlan) {
t.Errorf("Filter(%v):\n%s, want\n%s", tcase.input, gotPlan, wantPlan)
}
gotErr := ""
if err != nil {
gotErr = err.Error()
}
if gotErr != tcase.err {
t.Errorf("Filter err(%v): %s, want %v", tcase.input, gotErr, tcase.err)
}
require.Equal(t, tcase.err, gotErr, "Filter err(%v): %s, want %v", tcase.input, gotErr, tcase.err)
gotPlan, _ := json.Marshal(plan)
wantPlan, _ := json.Marshal(tcase.plan)
require.Equal(t, string(wantPlan), string(gotPlan), "Filter(%v):\n%s, want\n%s", tcase.input, gotPlan, wantPlan)

plan, err = buildReplicatorPlan(getSource(tcase.input), PrimaryKeyInfos, copyState, binlogplayer.NewStats())
if err != nil {
continue
}
gotPlan, _ = json.Marshal(plan)
wantPlan, _ = json.Marshal(tcase.planpk)
if string(gotPlan) != string(wantPlan) {
t.Errorf("Filter(%v,copyState):\n%s, want\n%s", tcase.input, gotPlan, wantPlan)
}
require.Equal(t, string(wantPlan), string(gotPlan), "Filter(%v,copyState):\n%s, want\n%s", tcase.input, gotPlan, wantPlan)
}
}

Expand Down
28 changes: 14 additions & 14 deletions go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ func buildTablePlan(tableName string, rule *binlogdatapb.Rule, colInfos []*Colum
// If it's a "select *", we return a partial plan, and complete
// it when we get back field info from the stream.
if len(sel.SelectExprs) != 1 {
return nil, fmt.Errorf("unexpected: %v", sqlparser.String(sel))
return nil, fmt.Errorf("unsupported mix of '*' and columns in query: %v", sqlparser.String(sel))
}
if !expr.TableName.IsEmpty() {
return nil, fmt.Errorf("unsupported qualifier for '*' expression: %v", sqlparser.String(expr))
Expand Down Expand Up @@ -377,25 +377,25 @@ func (tpb *tablePlanBuilder) generate() *TablePlan {
func analyzeSelectFrom(query string) (sel *sqlparser.Select, from string, err error) {
statement, err := sqlparser.Parse(query)
if err != nil {
return nil, "", err
return nil, "", vterrors.Wrapf(err, "failed to parse query %q", query)
}
sel, ok := statement.(*sqlparser.Select)
if !ok {
return nil, "", fmt.Errorf("unexpected: %v", sqlparser.String(statement))
return nil, "", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported non-select statement in query: %s", sqlparser.String(statement))
}
if sel.Distinct {
return nil, "", fmt.Errorf("unexpected: %v", sqlparser.String(sel))
return nil, "", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported distinct clause in query: %s", sqlparser.String(sel))
}
if len(sel.From) > 1 {
return nil, "", fmt.Errorf("unexpected: %v", sqlparser.String(sel))
return nil, "", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported multi-table query: %s", sqlparser.String(sel))
}
node, ok := sel.From[0].(*sqlparser.AliasedTableExpr)
if !ok {
return nil, "", fmt.Errorf("unexpected: %v", sqlparser.String(sel))
return nil, "", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported expression (%T) in from clause: %s", sel.From[0], sqlparser.String(sel))
}
fromTable := sqlparser.GetTableName(node.Expr)
if fromTable.IsEmpty() {
return nil, "", fmt.Errorf("unexpected: %v", sqlparser.String(sel))
return nil, "", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported from source (%T) in query: %s", node.Expr, sqlparser.String(sel))
}
return sel, fromTable.String(), nil
}
Expand All @@ -414,7 +414,7 @@ func (tpb *tablePlanBuilder) analyzeExprs(selExprs sqlparser.SelectExprs) error
func (tpb *tablePlanBuilder) analyzeExpr(selExpr sqlparser.SelectExpr) (*colExpr, error) {
aliased, ok := selExpr.(*sqlparser.AliasedExpr)
if !ok {
return nil, fmt.Errorf("unexpected: %v", sqlparser.String(selExpr))
return nil, fmt.Errorf("invalid expression: %v", sqlparser.String(selExpr))
}
as := aliased.As
if as.IsEmpty() {
Expand Down Expand Up @@ -463,7 +463,7 @@ func (tpb *tablePlanBuilder) analyzeExpr(selExpr sqlparser.SelectExpr) (*colExpr
switch fname := expr.Name.Lowered(); fname {
case "keyspace_id":
if len(expr.Exprs) != 0 {
return nil, fmt.Errorf("unexpected: %v", sqlparser.String(expr))
return nil, fmt.Errorf("unsupported multiple keyspace_id expressions: %v", sqlparser.String(expr))
}
tpb.sendSelect.SelectExprs = append(tpb.sendSelect.SelectExprs, &sqlparser.AliasedExpr{Expr: aliased.Expr})
// The vstreamer responds with "keyspace_id" as the field name for this request.
Expand All @@ -473,7 +473,7 @@ func (tpb *tablePlanBuilder) analyzeExpr(selExpr sqlparser.SelectExpr) (*colExpr
}
if expr, ok := aliased.Expr.(sqlparser.AggrFunc); ok {
if sqlparser.IsDistinct(expr) {
return nil, fmt.Errorf("unexpected: %v", sqlparser.String(expr))
return nil, fmt.Errorf("unsupported distinct expression usage: %v", sqlparser.String(expr))
}
switch fname := expr.AggrName(); fname {
case "count":
Expand All @@ -484,11 +484,11 @@ func (tpb *tablePlanBuilder) analyzeExpr(selExpr sqlparser.SelectExpr) (*colExpr
return cexpr, nil
case "sum":
if len(expr.GetArgs()) != 1 {
return nil, fmt.Errorf("unexpected: %v", sqlparser.String(expr))
return nil, fmt.Errorf("unsupported multiple columns in sum clause: %v", sqlparser.String(expr))
}
innerCol, ok := expr.GetArg().(*sqlparser.ColName)
if !ok {
return nil, fmt.Errorf("unexpected: %v", sqlparser.String(expr))
return nil, fmt.Errorf("unsupported non-column name in sum clause: %v", sqlparser.String(expr))
}
if !innerCol.Qualifier.IsEmpty() {
return nil, fmt.Errorf("unsupported qualifier for column: %v", sqlparser.String(innerCol))
Expand All @@ -511,7 +511,7 @@ func (tpb *tablePlanBuilder) analyzeExpr(selExpr sqlparser.SelectExpr) (*colExpr
case *sqlparser.Subquery:
return false, fmt.Errorf("unsupported subquery: %v", sqlparser.String(node))
case sqlparser.AggrFunc:
return false, fmt.Errorf("unexpected: %v", sqlparser.String(node))
return false, fmt.Errorf("unsupported aggregation function: %v", sqlparser.String(node))
}
return true, nil
}, aliased.Expr)
Expand All @@ -538,7 +538,7 @@ func (tpb *tablePlanBuilder) analyzeGroupBy(groupBy sqlparser.GroupBy) error {
for _, expr := range groupBy {
colname, ok := expr.(*sqlparser.ColName)
if !ok {
return fmt.Errorf("unexpected: %v", sqlparser.String(expr))
return fmt.Errorf("unsupported non-column name or alias in group by clause: %v", sqlparser.String(expr))
}
cexpr := tpb.findCol(colname.Name)
if cexpr == nil {
Expand Down

0 comments on commit 249a3f8

Please sign in to comment.