From c5e45299f5374adf0908f5d9a9ae107f5fc774be Mon Sep 17 00:00:00 2001 From: elianddb Date: Wed, 30 Jul 2025 21:53:43 +0000 Subject: [PATCH 1/2] Fix ALTER TABLE MODIFY COLUMN to report actual row count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Updated modifyColumnIter.Next() to count and return actual rows affected - Modified rewriteTable() signature to return row count (bool, int, error) - Added countTableRows() helper function for accurate row counting - Handles both rewrite and non-rewrite code paths correctly Fixes dolthub/dolt#9606 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- sql/rowexec/ddl_iters.go | 71 ++++++++++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 17 deletions(-) diff --git a/sql/rowexec/ddl_iters.go b/sql/rowexec/ddl_iters.go index 84da759376..8d0da3713e 100644 --- a/sql/rowexec/ddl_iters.go +++ b/sql/rowexec/ddl_iters.go @@ -352,12 +352,12 @@ func (i *modifyColumnIter) Next(ctx *sql.Context) (sql.Row, error) { // TODO: replace with different node in analyzer if rwt, ok := i.alterable.(sql.RewritableTable); ok { - rewritten, err := i.rewriteTable(ctx, rwt) + rewritten, rowCount, err := i.rewriteTable(ctx, rwt) if err != nil { return nil, err } if rewritten { - return sql.NewRow(types.NewOkResult(0)), nil + return sql.NewRow(types.NewOkResult(rowCount)), nil } } @@ -376,7 +376,42 @@ func (i *modifyColumnIter) Next(ctx *sql.Context) (sql.Row, error) { return nil, err } } - return sql.NewRow(types.NewOkResult(0)), nil + + rowCount, err := countTableRows(ctx, i.alterable) + if err != nil { + return nil, err + } + + return sql.NewRow(types.NewOkResult(rowCount)), nil +} + +// countTableRows counts the number of rows in a table for DDL result reporting +func countTableRows(ctx *sql.Context, table sql.Table) (int, error) { + partitions, err := table.Partitions(ctx) + if err != nil { + return 0, err + } + + rowIter := sql.NewTableRowIter(ctx, table, partitions) + defer func() { + if rowIter != nil { + _ = rowIter.Close(ctx) + } + }() + + var count int + for { + _, err := rowIter.Next(ctx) + if err == io.EOF { + break + } + if err != nil { + return 0, err + } + count++ + } + + return count, nil } func handleFkColumnRename(ctx *sql.Context, fkTable sql.ForeignKeyTable, db sql.Database, oldName string, newName string) error { @@ -481,20 +516,20 @@ func (i *modifyColumnIter) Close(context *sql.Context) error { } // rewriteTable rewrites the table given if required or requested, and returns whether it was rewritten -func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTable) (bool, error) { +func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTable) (bool, int, error) { targetSchema := i.m.TargetSchema() oldColName := i.m.Column() oldColIdx := targetSchema.IndexOfColName(oldColName) if oldColIdx == -1 { // Should be impossible, checked in analyzer - return false, sql.ErrTableColumnNotFound.New(rwt.Name(), oldColName) + return false, 0, sql.ErrTableColumnNotFound.New(rwt.Name(), oldColName) } oldCol := i.m.TargetSchema()[oldColIdx] newCol := i.m.NewColumn() newSch, projections, err := modifyColumnInSchema(targetSchema, oldColName, newCol, i.m.Order()) if err != nil { - return false, err + return false, 0, err } // Wrap any auto increment columns in auto increment expressions. This mirrors what happens to row sources for normal @@ -503,7 +538,7 @@ func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTabl if col.AutoIncrement { projections[i], err = expression.NewAutoIncrementForColumn(ctx, rwt, col, projections[i]) if err != nil { - return false, err + return false, 0, err } } } @@ -532,20 +567,21 @@ func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTabl // TODO: codify rewrite requirements rewriteRequested := rwt.ShouldRewriteTable(ctx, oldPkSchema, newPkSchema, oldCol, newCol) if !rewriteRequired && !rewriteRequested { - return false, nil + return false, 0, nil } inserter, err := rwt.RewriteInserter(ctx, oldPkSchema, newPkSchema, oldCol, newCol, nil) if err != nil { - return false, err + return false, 0, err } partitions, err := rwt.Partitions(ctx) if err != nil { - return false, err + return false, 0, err } rowIter := sql.NewTableRowIter(ctx, rwt, partitions) + var rowCount int for { r, err := rowIter.Next(ctx) if err == io.EOF { @@ -553,7 +589,7 @@ func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTabl } else if err != nil { _ = inserter.DiscardChanges(ctx, err) _ = inserter.Close(ctx) - return false, err + return false, 0, err } // remap old enum values to new enum values @@ -564,7 +600,7 @@ func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTabl oldStr, _ := oldEnum.At(oldIdx) newIdx := newEnum.IndexOf(oldStr) if newIdx == -1 { - return false, types.ErrDataTruncatedForColumn.New(newCol.Name) + return false, 0, types.ErrDataTruncatedForColumn.New(newCol.Name) } r[oldColIdx] = uint16(newIdx) } @@ -574,31 +610,32 @@ func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTabl if err != nil { _ = inserter.DiscardChanges(ctx, err) _ = inserter.Close(ctx) - return false, err + return false, 0, err } err = i.validateNullability(ctx, newSch, newRow) if err != nil { _ = inserter.DiscardChanges(ctx, err) _ = inserter.Close(ctx) - return false, err + return false, 0, err } err = inserter.Insert(ctx, newRow) if err != nil { _ = inserter.DiscardChanges(ctx, err) _ = inserter.Close(ctx) - return false, err + return false, 0, err } + rowCount++ } // TODO: move this into iter.close, probably err = inserter.Close(ctx) if err != nil { - return false, err + return false, 0, err } - return true, nil + return true, rowCount, nil } // modifyColumnInSchema modifies the given column in given schema and returns the new schema, along with a set of From 457291111e525f6da1f2d6a28d83722c64c235f0 Mon Sep 17 00:00:00 2001 From: elianddb Date: Wed, 30 Jul 2025 15:35:55 -0700 Subject: [PATCH 2/2] amend existing memoryengine tests --- enginetest/enginetests.go | 2 +- enginetest/queries/alter_table_queries.go | 28 +++++++++---------- .../queries/charset_collation_engine.go | 4 +-- enginetest/queries/foreign_key_queries.go | 8 +++--- enginetest/queries/fulltext_queries.go | 4 +-- enginetest/queries/script_queries.go | 12 ++++---- enginetest/queries/update_queries.go | 2 +- 7 files changed, 30 insertions(+), 30 deletions(-) diff --git a/enginetest/enginetests.go b/enginetest/enginetests.go index 5691405a1f..26b9581d2c 100644 --- a/enginetest/enginetests.go +++ b/enginetest/enginetests.go @@ -3145,7 +3145,7 @@ func TestModifyColumn(t *testing.T, harness Harness) { se.NewConnection(ctx) } TestQueryWithContext(t, ctx, e, harness, "select database()", []sql.Row{{nil}}, nil, nil, nil) - TestQueryWithContext(t, ctx, e, harness, "ALTER TABLE mydb.mytable MODIFY COLUMN s VARCHAR(21) NULL COMMENT 'changed again'", []sql.Row{{types.NewOkResult(0)}}, nil, nil, nil) + TestQueryWithContext(t, ctx, e, harness, "ALTER TABLE mydb.mytable MODIFY COLUMN s VARCHAR(21) NULL COMMENT 'changed again'", []sql.Row{{types.NewOkResult(4)}}, nil, nil, nil) TestQueryWithContext(t, ctx, e, harness, "SHOW FULL COLUMNS FROM mydb.mytable", []sql.Row{ {"i", "bigint", nil, "NO", "PRI", nil, "", "", "ok"}, {"s", "varchar(21)", "utf8mb4_0900_bin", "YES", "", nil, "", "", "changed again"}, diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index bec63ab273..95cef69c88 100644 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -456,7 +456,7 @@ var AlterTableScripts = []ScriptTest{ Assertions: []ScriptTestAssertion{ { Query: "ALTER TABLE t40 MODIFY COLUMN pk int", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(3)}}, }, { Query: "describe t40", @@ -1091,7 +1091,7 @@ var AlterTableScripts = []ScriptTest{ Assertions: []ScriptTestAssertion{ { Query: `alter table t modify column c1 int unsigned`, - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "describe t;", @@ -1156,7 +1156,7 @@ var AlterTableScripts = []ScriptTest{ { Query: "alter table t modify column e enum('c', 'a', 'b');", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(4)}, }, }, { @@ -1171,7 +1171,7 @@ var AlterTableScripts = []ScriptTest{ { Query: "alter table t modify column e enum('asdf', 'a', 'b', 'c');", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(4)}, }, }, { @@ -1186,7 +1186,7 @@ var AlterTableScripts = []ScriptTest{ { Query: "alter table t modify column e enum('asdf', 'a', 'b', 'c', 'd');", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(4)}, }, }, { @@ -1201,7 +1201,7 @@ var AlterTableScripts = []ScriptTest{ { Query: "alter table t modify column e enum('a', 'b', 'c');", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(4)}, }, }, { @@ -1244,7 +1244,7 @@ var AlterTableScripts = []ScriptTest{ { Query: "alter table t modify column s set('a', 'b', 'c', 'd');", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(8)}, }, }, { @@ -2166,7 +2166,7 @@ var ModifyColumnScripts = []ScriptTest{ Assertions: []ScriptTestAssertion{ { Query: "ALTER TABLE mytable MODIFY COLUMN i bigint NOT NULL COMMENT 'modified'", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(3)}}, }, { Query: "SHOW FULL COLUMNS FROM mytable /* 1 */", @@ -2177,7 +2177,7 @@ var ModifyColumnScripts = []ScriptTest{ }, { Query: "ALTER TABLE mytable MODIFY COLUMN i TINYINT NOT NULL COMMENT 'yes' AFTER s", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(3)}}, }, { Query: "SHOW FULL COLUMNS FROM mytable /* 2 */", @@ -2188,7 +2188,7 @@ var ModifyColumnScripts = []ScriptTest{ }, { Query: "ALTER TABLE mytable MODIFY COLUMN i BIGINT NOT NULL COMMENT 'ok' FIRST", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(3)}}, }, { Query: "SHOW FULL COLUMNS FROM mytable /* 3 */", @@ -2199,7 +2199,7 @@ var ModifyColumnScripts = []ScriptTest{ }, { Query: "ALTER TABLE mytable MODIFY COLUMN s VARCHAR(20) NULL COMMENT 'changed'", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(3)}}, }, { Query: "SHOW FULL COLUMNS FROM mytable /* 4 */", @@ -2216,7 +2216,7 @@ var ModifyColumnScripts = []ScriptTest{ Assertions: []ScriptTestAssertion{ { Query: "ALTER TABLE mytable MODIFY i BIGINT auto_increment", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(3)}}, }, { Query: "SHOW FULL COLUMNS FROM mytable /* 1 */", @@ -2249,7 +2249,7 @@ var ModifyColumnScripts = []ScriptTest{ }, { Query: "ALTER TABLE mytable MODIFY COLUMN i BIGINT NOT NULL COMMENT 'ok' FIRST", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(4)}}, }, { Query: "SHOW FULL COLUMNS FROM mytable /* 3 */", @@ -2261,7 +2261,7 @@ var ModifyColumnScripts = []ScriptTest{ }, { Query: "ALTER TABLE mytable MODIFY COLUMN s VARCHAR(20) NULL COMMENT 'changed'", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(4)}}, }, { Query: "SHOW FULL COLUMNS FROM mytable /* 4 */", diff --git a/enginetest/queries/charset_collation_engine.go b/enginetest/queries/charset_collation_engine.go index 8c5b8a6278..92a0e31e27 100644 --- a/enginetest/queries/charset_collation_engine.go +++ b/enginetest/queries/charset_collation_engine.go @@ -282,7 +282,7 @@ var CharsetCollationEngineTests = []CharsetCollationEngineTest{ { Query: "ALTER TABLE test2 MODIFY COLUMN v1 VARCHAR(100);", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(4)}, }, }, { @@ -342,7 +342,7 @@ var CharsetCollationEngineTests = []CharsetCollationEngineTest{ { Query: "ALTER TABLE test2 CHANGE COLUMN v1 v1 VARCHAR(220);", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(4)}, }, }, { diff --git a/enginetest/queries/foreign_key_queries.go b/enginetest/queries/foreign_key_queries.go index c94d162c38..c1ce4ad4bc 100644 --- a/enginetest/queries/foreign_key_queries.go +++ b/enginetest/queries/foreign_key_queries.go @@ -513,11 +513,11 @@ var ForeignKeyTests = []ScriptTest{ }, { Query: "ALTER TABLE parent2 MODIFY v1 VARCHAR(30);", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(2)}}, }, { Query: "ALTER TABLE child2 MODIFY v1 VARCHAR(30);", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "ALTER TABLE parent3 MODIFY v1 BINARY(30);", @@ -564,11 +564,11 @@ var ForeignKeyTests = []ScriptTest{ }, { Query: "ALTER TABLE parent1 MODIFY v2 BIGINT;", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(2)}}, }, { Query: "ALTER TABLE child1 MODIFY v2 BIGINT;", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, }, }, diff --git a/enginetest/queries/fulltext_queries.go b/enginetest/queries/fulltext_queries.go index 23276a6287..934d193db2 100644 --- a/enginetest/queries/fulltext_queries.go +++ b/enginetest/queries/fulltext_queries.go @@ -733,7 +733,7 @@ var FulltextTests = []ScriptTest{ Assertions: []ScriptTestAssertion{ { Query: "ALTER TABLE test MODIFY COLUMN v3 FLOAT AFTER pk;", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(5)}}, }, { Query: "SELECT * FROM test WHERE MATCH(v1, v2) AGAINST ('ghi');", @@ -751,7 +751,7 @@ var FulltextTests = []ScriptTest{ Assertions: []ScriptTestAssertion{ { Query: "ALTER TABLE test MODIFY COLUMN v2 TEXT;", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(5)}}, }, { Query: "SELECT * FROM test WHERE MATCH(v1, v2) AGAINST ('ghi');", diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go index 47c2005018..1145618df1 100644 --- a/enginetest/queries/script_queries.go +++ b/enginetest/queries/script_queries.go @@ -3722,7 +3722,7 @@ CREATE TABLE tab3 ( }, { Query: "ALTER TABLE TEST MODIFY COLUMN pk BIGINT AUTO_INCREMENT, AUTO_INCREMENT = 100", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "INSERT INTO test (v2) values (11)", @@ -11453,7 +11453,7 @@ var SpatialScriptTests = []ScriptTest{ }, { Query: "ALTER TABLE tab2 CHANGE COLUMN p p POINT NOT NULL", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "INSERT INTO tab2 VALUES (2, ST_GEOMFROMTEXT(ST_ASWKT(POINT(1, 6)), 4326))", @@ -11473,7 +11473,7 @@ var SpatialScriptTests = []ScriptTest{ }, { Query: "ALTER TABLE tab2 CHANGE COLUMN p p POINT NOT NULL SRID 4326", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "show create table tab2", @@ -11501,7 +11501,7 @@ var SpatialScriptTests = []ScriptTest{ }, { Query: "ALTER TABLE tab3 MODIFY COLUMN y POLYGON NOT NULL SRID 0", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "ALTER TABLE tab3 MODIFY COLUMN y POLYGON NOT NULL SRID 4326", @@ -11513,7 +11513,7 @@ var SpatialScriptTests = []ScriptTest{ }, { Query: "ALTER TABLE tab3 MODIFY COLUMN y GEOMETRY NULL SRID 0", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "select i, ST_ASWKT(y) FROM tab3", @@ -11565,7 +11565,7 @@ var SpatialScriptTests = []ScriptTest{ }, { Query: "ALTER TABLE table1 CHANGE COLUMN p p geometry srid 4326", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Expected: []sql.Row{{types.NewOkResult(1)}}, }, { Query: "show create table table1", diff --git a/enginetest/queries/update_queries.go b/enginetest/queries/update_queries.go index 28490233c7..f9a3f3c1bf 100644 --- a/enginetest/queries/update_queries.go +++ b/enginetest/queries/update_queries.go @@ -1342,7 +1342,7 @@ var OnUpdateExprScripts = []ScriptTest{ { Query: "alter table t modify column ts timestamp default 0 on update current_timestamp;", Expected: []sql.Row{ - {types.NewOkResult(0)}, + {types.NewOkResult(3)}, }, }, {