Changes after self review

Signed-off-by: Matt Lord <[email protected]>
vitessio · Aug 20, 2024 · f98d261 · f98d261
1 parent 56ef500
commit f98d261
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 38 deletions.
diff --git a/go/vt/vttablet/tabletmanager/vdiff/framework_test.go b/go/vt/vttablet/tabletmanager/vdiff/framework_test.go
@@ -662,14 +662,14 @@ func (tvde *testVDiffEnv) addTablet(id int, keyspace, shard string, tabletType t
 	return tvde.tablets[id]
 }
 
-func (tvde *testVDiffEnv) createController(t *testing.T) *controller {
+func (tvde *testVDiffEnv) createController(t *testing.T, id int) *controller {
 	controllerQR := sqltypes.MakeTestResult(sqltypes.MakeTestFields(
 		vdiffTestCols,
 		vdiffTestColTypes,
 	),
-		fmt.Sprintf("1|%s|%s|%s|%s|%s|%s|%s|", uuid.New(), tvde.workflow, tstenv.KeyspaceName, tstenv.ShardName, vdiffDBName, PendingState, optionsJS),
+		fmt.Sprintf("%d|%s|%s|%s|%s|%s|%s|%s|", id, uuid.New(), tvde.workflow, tstenv.KeyspaceName, tstenv.ShardName, vdiffDBName, PendingState, optionsJS),
 	)
-	tvde.dbClient.ExpectRequest("select * from _vt.vdiff where id = 1", noResults, nil)
+	tvde.dbClient.ExpectRequest(fmt.Sprintf("select * from _vt.vdiff where id = %d", id), noResults, nil)
 	ct, err := newController(context.Background(), controllerQR.Named().Row(), tvde.dbClientFactory, tstenv.TopoServ, tvde.vde, tvde.opts)
 	require.NoError(t, err)
 	return ct

diff --git a/go/vt/vttablet/tabletmanager/vdiff/report.go b/go/vt/vttablet/tabletmanager/vdiff/report.go
@@ -63,7 +63,7 @@ type RowDiff struct {
 	Query string            `json:"Query,omitempty"`
 }
 
-func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, reportOptions *tabletmanagerdatapb.VDiffReportOptions) (*RowDiff, error) {
+func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, opts *tabletmanagerdatapb.VDiffReportOptions) (*RowDiff, error) {
 	rd := &RowDiff{}
 	rd.Row = make(map[string]string)
 	statement, err := td.wd.ct.vde.parser.Parse(queryStmt)
@@ -75,8 +75,8 @@ func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, report
 		return nil, fmt.Errorf("unexpected: %+v", sqlparser.String(statement))
 	}
 
-	if reportOptions.GetDebugQuery() {
-		rd.Query = td.genDebugQueryDiff(sel, row, reportOptions.GetOnlyPks())
+	if opts.GetDebugQuery() {
+		rd.Query = td.genDebugQueryDiff(sel, row, opts.GetOnlyPks())
 	}
 
 	addVal := func(index int, truncateAt int) {
@@ -101,14 +101,14 @@ func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, report
 		pks[pkI] = struct{}{}
 	}
 
-	if reportOptions.GetOnlyPks() {
+	if opts.GetOnlyPks() {
 		return rd, nil
 	}
 
-	rowDiffColumnTruncateAt := int(reportOptions.GetRowDiffColumnTruncateAt())
+	truncateAt := int(opts.GetRowDiffColumnTruncateAt())
 	for i := range sel.SelectExprs {
 		if _, pk := pks[i]; !pk {
-			addVal(i, rowDiffColumnTruncateAt)
+			addVal(i, truncateAt)
 		}
 	}
 

diff --git a/go/vt/vttablet/tabletmanager/vdiff/report_test.go b/go/vt/vttablet/tabletmanager/vdiff/report_test.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2022 The Vitess Authors.
+Copyright 2024 The Vitess Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -40,32 +40,34 @@ func TestGenRowDiff(t *testing.T) {
 		row           []sqltypes.Value
 		reportOptions *tabletmanagerdatapb.VDiffReportOptions
 		want          *RowDiff
-		wantErr       bool
 	}{
 		{
 			name: "defaults",
 			schema: &tabletmanagerdatapb.SchemaDefinition{
 				TableDefinitions: []*tabletmanagerdatapb.TableDefinition{
 					{
 						Name:              "t1",
-						Columns:           []string{"c1", "c2"},
-						PrimaryKeyColumns: []string{"c1"},
-						Fields:            sqltypes.MakeTestFields("c1|c2", "int64|int64"),
+						Columns:           []string{"c1", "c2", "c3", "c4", "c5"},
+						PrimaryKeyColumns: []string{"c1", "c5"},
+						Fields:            sqltypes.MakeTestFields("c1|c2|c3|c4|c5", "int64|int64|varchar|varchar|int64"),
 					},
 				},
 			},
-			query: "select c1,c2 from t1",
+			query: "select c1,c2,c3,c4,c5 from t1",
 			tablePlan: &tablePlan{
-				selectPks: []int{0},
+				selectPks: []int{0, 4},
 			},
 			row: []sqltypes.Value{
 				sqltypes.NewInt64(1),
 				sqltypes.NewInt64(2),
+				sqltypes.NewVarChar("hi3"),
+				sqltypes.NewVarChar("hi4"),
+				sqltypes.NewInt64(5),
 			},
 			reportOptions: &tabletmanagerdatapb.VDiffReportOptions{},
 			want: &RowDiff{
-				Row: map[string]string{
-					"c1": "1", "c2": "2",
+				Row: map[string]string{ // The two PK cols should be first
+					"c1": "1", "c5": "5", "c2": "2", "c3": "hi3", "c4": "hi4",
 				},
 			},
 		},
@@ -167,23 +169,18 @@ func TestGenRowDiff(t *testing.T) {
 			require.NotNil(t, tc.reportOptions)
 
 			vdenv.tmc.schema = tc.schema
-			ct := vdenv.createController(t)
+			ct := vdenv.createController(t, 1)
 			wd, err := newWorkflowDiffer(ct, vdenv.opts, collations.MySQL8())
 			require.NoError(t, err)
-
 			td := &tableDiffer{
 				wd:          wd,
 				sourceQuery: tc.query,
 				tablePlan:   tc.tablePlan,
 			}
+
 			got, err := td.genRowDiff(tc.query, tc.row, tc.reportOptions)
-			if tc.wantErr {
-				require.Error(t, err, "tableDiffer.genRowDiff() error = %v, wantErr %v",
-					err, tc.wantErr)
-				return
-			} else {
-				require.EqualValues(t, tc.want, got)
-			}
+			require.NoError(t, err)
+			require.EqualValues(t, tc.want, got)
 		})
 	}
 }
diff --git a/go/vt/vttablet/tabletmanager/vdiff/table_differ.go b/go/vt/vttablet/tabletmanager/vdiff/table_differ.go
@@ -486,7 +486,7 @@ func (td *tableDiffer) setupRowSorters() {
 	}
 }
 
-func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatapb.VDiffCoreOptions, reportOptions *tabletmanagerdatapb.VDiffReportOptions, stop <-chan time.Time) (*DiffReport, error) {
+func (td *tableDiffer) diff(ctx context.Context, coreOpts *tabletmanagerdatapb.VDiffCoreOptions, reportOpts *tabletmanagerdatapb.VDiffReportOptions, stop <-chan time.Time) (*DiffReport, error) {
 	defer td.wd.ct.TableDiffPhaseTimings.Record(fmt.Sprintf("%s.%s", td.table.Name, diffingTable), time.Now())
 	dbClient := td.wd.ct.dbClientFactory()
 	if err := dbClient.Connect(); err != nil {
@@ -539,9 +539,9 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap
 		globalStats.RowsDiffedCount.Add(dr.ProcessedRows)
 	}()
 
-	rowsToCompare := coreOptions.GetMaxRows()
-	maxExtraRowsToCompare := coreOptions.GetMaxExtraRowsToCompare()
-	maxReportSampleRows := reportOptions.GetMaxSampleRows()
+	rowsToCompare := coreOpts.GetMaxRows()
+	maxExtraRowsToCompare := coreOpts.GetMaxExtraRowsToCompare()
+	maxReportSampleRows := reportOpts.GetMaxSampleRows()
 
 	for {
 		lastProcessedRow = sourceRow
@@ -592,7 +592,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap
 		advanceSource = true
 		advanceTarget = true
 		if sourceRow == nil {
-			diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, targetRow, reportOptions)
+			diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, targetRow, reportOpts)
 			if err != nil {
 				return nil, vterrors.Wrap(err, "unexpected error generating diff")
 			}
@@ -610,7 +610,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap
 		if targetRow == nil {
 			// No more rows from the target but we know we have more rows from
 			// source, so drain them and update the counts.
-			diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOptions)
+			diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOpts)
 			if err != nil {
 				return nil, vterrors.Wrap(err, "unexpected error generating diff")
 			}
@@ -633,7 +633,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap
 			return nil, err
 		case c < 0:
 			if dr.ExtraRowsSource < maxExtraRowsToCompare {
-				diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOptions)
+				diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOpts)
 				if err != nil {
 					return nil, vterrors.Wrap(err, "unexpected error generating diff")
 				}
@@ -644,7 +644,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap
 			continue
 		case c > 0:
 			if dr.ExtraRowsTarget < maxExtraRowsToCompare {
-				diffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOptions)
+				diffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOpts)
 				if err != nil {
 					return nil, vterrors.Wrap(err, "unexpected error generating diff")
 				}
@@ -664,11 +664,11 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap
 		case c != 0:
 			// We don't do a second pass to compare mismatched rows so we can cap the slice here.
 			if maxReportSampleRows == 0 || dr.MismatchedRows < maxReportSampleRows {
-				sourceDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, sourceRow, reportOptions)
+				sourceDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, sourceRow, reportOpts)
 				if err != nil {
 					return nil, vterrors.Wrap(err, "unexpected error generating diff")
 				}
-				targetDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOptions)
+				targetDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOpts)
 				if err != nil {
 					return nil, vterrors.Wrap(err, "unexpected error generating diff")
 				}

diff --git a/go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go b/go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go
@@ -588,7 +588,7 @@ func TestBuildPlanInclude(t *testing.T) {
 	vdenv := newTestVDiffEnv(t)
 	defer vdenv.close()
 
-	ct := vdenv.createController(t)
+	ct := vdenv.createController(t, 1)
 
 	schm := &tabletmanagerdatapb.SchemaDefinition{
 		TableDefinitions: []*tabletmanagerdatapb.TableDefinition{{