From 249a3f8c41bcd8c2318966b65412a022725c3b0d Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Mon, 11 Dec 2023 17:41:26 -0500 Subject: [PATCH] Improve replication plan error messages Signed-off-by: Matt Lord --- .../vreplication/replicator_plan_test.go | 47 +++++++++---------- .../vreplication/table_plan_builder.go | 28 +++++------ 2 files changed, 35 insertions(+), 40 deletions(-) diff --git a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan_test.go b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan_test.go index 780b1c0d064..bda48b72eaf 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan_test.go +++ b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan_test.go @@ -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" ) @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -734,18 +735,14 @@ 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 { @@ -753,9 +750,7 @@ func TestBuildPlayerPlan(t *testing.T) { } 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) } } diff --git a/go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go b/go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go index 715d87186a6..219064399e4 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go +++ b/go/vt/vttablet/tabletmanager/vreplication/table_plan_builder.go @@ -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)) @@ -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 } @@ -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() { @@ -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. @@ -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": @@ -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)) @@ -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) @@ -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 {