Skip to content

Commit e4d359d

Browse files
committed
Addressed some Copilot findings
1 parent 2b84cc4 commit e4d359d

4 files changed

Lines changed: 80 additions & 1 deletion

File tree

go/logic/applier.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1218,7 +1218,7 @@ func (apl *Applier) ApplyIterationInsertQuery() (chunkSize int64, rowsAffected i
12181218
}
12191219

12201220
// ApplyIterationMoveTableCopyQueries issues a SELECT query on the original table and an INSERT query on the target table,
1221-
// copying a chunk of rows. It is used when `--move-table` is specified, instead of ApplyIterationInsertQuery.
1221+
// copying a chunk of rows. It is used when `--move-tables` is specified, instead of ApplyIterationInsertQuery.
12221222
func (apl *Applier) ApplyIterationMoveTableCopyQueries() (chunkSize int64, rowsAffected int64, duration time.Duration, err error) {
12231223
startTime := time.Now()
12241224
chunkSize = atomic.LoadInt64(&apl.migrationContext.ChunkSize)
@@ -1252,12 +1252,21 @@ func (apl *Applier) ApplyIterationMoveTableCopyQueries() (chunkSize int64, rowsA
12521252
}
12531253
chunkRows = append(chunkRows, row)
12541254
}
1255+
if rowsErr := sqlRows.Err(); rowsErr != nil {
1256+
return nil, rowsErr
1257+
}
12551258
return chunkRows, nil
12561259
}()
12571260
if err != nil {
12581261
return chunkSize, rowsAffected, duration, err
12591262
}
12601263

1264+
// no need to INSERT if there are no rows to copy:
1265+
if len(rows) == 0 {
1266+
duration = time.Since(startTime)
1267+
return chunkSize, 0, duration, nil
1268+
}
1269+
12611270
// Then, insert data into the destination database:
12621271
sqlResult, err := func() (gosql.Result, error) {
12631272
query, explodedArgs, err := apl.moveTablesCopyInsertQueryBuilder.BuildQuery(rows)

go/logic/applier_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,7 @@ func (suite *ApplierTestSuite) SetupSuite() {
302302

303303
// Second database & connection for move-tables tests:
304304
_, err = suite.db.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s", testMysqlDatabaseOther))
305+
suite.Require().NoError(err)
305306
otherConf := drivermysql.NewConfig()
306307
otherConf.DBName = testMysqlDatabaseOther
307308
otherConf.User = testMysqlUser
@@ -1789,6 +1790,59 @@ func (suite *ApplierTestSuite) TestApplyIterationMoveTableCopyQueries() {
17891790
suite.Require().Equal("2025-12-31 23:59:59", results[2].createdAt)
17901791
}
17911792

1793+
func (suite *ApplierTestSuite) TestApplyIterationMoveTableCopyQueriesNoRows() {
1794+
ctx := context.Background()
1795+
var err error
1796+
1797+
_, err = suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT NOT NULL, name VARCHAR(50), created_at DATETIME NOT NULL, PRIMARY KEY(id));", getTestTableName()))
1798+
suite.Require().NoError(err)
1799+
_, err = suite.otherDB.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT NOT NULL, name VARCHAR(50), created_at DATETIME NOT NULL, PRIMARY KEY(id));", getTestOtherTableName()))
1800+
suite.Require().NoError(err)
1801+
_, err = suite.db.ExecContext(ctx, fmt.Sprintf("INSERT INTO %s (id, name, created_at) VALUES (1, 'alice', '2024-01-15 10:30:00'), (2, 'bob', '2024-06-20 14:45:00'), (3, 'carol', '2025-12-31 23:59:59');", getTestTableName()))
1802+
suite.Require().NoError(err)
1803+
1804+
connectionConfig, err := getTestConnectionConfig(ctx, suite.mysqlContainer)
1805+
suite.Require().NoError(err)
1806+
1807+
migrationContext := newTestMigrationContext()
1808+
migrationContext.ApplierConnectionConfig = connectionConfig
1809+
migrationContext.MoveTables.ConnectionConfig = connectionConfig
1810+
migrationContext.SetConnectionConfig("innodb")
1811+
migrationContext.OriginalTableColumns = sql.NewColumnList([]string{"id", "name", "created_at"})
1812+
migrationContext.SharedColumns = sql.NewColumnList([]string{"id", "name", "created_at"})
1813+
migrationContext.MappedSharedColumns = sql.NewColumnList([]string{"id", "name", "created_at"})
1814+
migrationContext.UniqueKey = &sql.UniqueKey{
1815+
Name: "PRIMARY",
1816+
Columns: *sql.NewColumnList([]string{"id"}),
1817+
}
1818+
migrationContext.MoveTables.TableNames = []string{testMysqlTableName}
1819+
migrationContext.MoveTables.TargetDatabase = testMysqlDatabaseOther
1820+
1821+
applier := NewApplier(migrationContext)
1822+
applier.prepareQueries()
1823+
defer applier.Teardown()
1824+
1825+
err = applier.InitDBConnections()
1826+
suite.Require().NoError(err)
1827+
1828+
// Point the iteration range at a key range that contains no rows so the
1829+
// SELECT returns an empty result set and the INSERT is skipped.
1830+
migrationContext.MigrationIterationRangeMinValues = sql.ToColumnValues([]interface{}{100})
1831+
migrationContext.MigrationIterationRangeMaxValues = sql.ToColumnValues([]interface{}{200})
1832+
1833+
chunkSize, rowsAffected, duration, err := applier.ApplyIterationMoveTableCopyQueries()
1834+
suite.Require().NoError(err)
1835+
suite.Require().Equal(int64(0), rowsAffected)
1836+
suite.Require().Equal(int64(1000), chunkSize)
1837+
suite.Require().Greater(duration, time.Duration(0))
1838+
1839+
// Verify no rows were copied to the target table.
1840+
var count int
1841+
err = suite.otherDB.QueryRowContext(ctx, "SELECT COUNT(*) FROM "+getTestOtherTableName()).Scan(&count)
1842+
suite.Require().NoError(err)
1843+
suite.Require().Equal(0, count)
1844+
}
1845+
17921846
func TestApplier(t *testing.T) {
17931847
if testing.Short() {
17941848
t.Skip("skipping applier test suite in short mode")

go/sql/builder.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,9 @@ func (b *MoveTableCopySelectQueryBuilder) BuildQuery(rangeStartArgs, rangeEndArg
501501
if len(rangeStartArgs)+len(rangeEndArgs) != b.argsCount {
502502
return "", nil, fmt.Errorf("got %d args but expected %d", len(rangeStartArgs)+len(rangeEndArgs), b.argsCount)
503503
}
504+
if len(rangeStartArgs) != len(rangeEndArgs) {
505+
return "", nil, fmt.Errorf("mismatched number of start and end args: %d != %d", len(rangeStartArgs), len(rangeEndArgs))
506+
}
504507
explodedArgs := make([]any, 0, len(b.argsMapping))
505508
for _, idx := range b.argsMapping {
506509
if idx < len(rangeStartArgs) {

go/sql/builder_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1197,6 +1197,19 @@ func TestMoveTableCopySelectQueryBuilder(t *testing.T) {
11971197
_, _, err = builder.BuildQuery([]any{1, 2}, []any{10})
11981198
require.Error(t, err)
11991199
})
1200+
1201+
t.Run("mismatched start and end args count", func(t *testing.T) {
1202+
sharedColumns := NewColumnList([]string{"id", "name", "position"})
1203+
uniqueKeyColumns := NewColumnList([]string{"name", "position"})
1204+
1205+
builder, err := NewMoveTableCopySelectQueryBuilder("mydb", "tbl", sharedColumns, "name_position_uidx", uniqueKeyColumns, true)
1206+
require.NoError(t, err)
1207+
1208+
// Total args count matches argsCount (4), but start and end counts differ.
1209+
_, _, err = builder.BuildQuery([]any{1, 2, 3}, []any{10})
1210+
require.Error(t, err)
1211+
require.Contains(t, err.Error(), "mismatched number of start and end args")
1212+
})
12001213
}
12011214

12021215
func BenchmarkMoveTableCopySelectQueryBuilderBuildQuery(b *testing.B) {

0 commit comments

Comments
 (0)