From ea49e655e24b1e6e956bdb010474183d01a39172 Mon Sep 17 00:00:00 2001 From: Neil Macneale IV Date: Mon, 13 Jan 2025 18:08:56 -0800 Subject: [PATCH 1/4] Don't panic when attempting to update workspace table with schema changes --- .../doltcore/sqle/dtables/workspace_table.go | 53 ++++++++++++++----- .../sqle/enginetest/dolt_queries_workspace.go | 43 ++++++++++++++- 2 files changed, 82 insertions(+), 14 deletions(-) diff --git a/go/libraries/doltcore/sqle/dtables/workspace_table.go b/go/libraries/doltcore/sqle/dtables/workspace_table.go index 7a409356e4..f4b52058f0 100644 --- a/go/libraries/doltcore/sqle/dtables/workspace_table.go +++ b/go/libraries/doltcore/sqle/dtables/workspace_table.go @@ -62,12 +62,17 @@ type WorkspaceTable struct { // headSchema is the schema of the table that is being modified. headSchema schema.Schema + + // modifiable is true if the schemas before and after the update are identical. Used to reject updates that would + // be impossible perform - such as only stage one row when the entire schema of the table is being modified. + modifiable bool } type WorkspaceTableModifier struct { - tableName doltdb.TableName - ws *doltdb.WorkingSet - head doltdb.RootValue + tableName doltdb.TableName + workspaceTableName string + ws *doltdb.WorkingSet + head doltdb.RootValue headSch schema.Schema schemaLen int @@ -76,6 +81,9 @@ type WorkspaceTableModifier struct { tableWriter *dsess.TableWriter sessionWriter *dsess.WriteSession + // modifiable carried through from the main table. + modifiable bool + err *error } @@ -151,6 +159,10 @@ func (wtu *WorkspaceTableUpdater) Update(ctx *sql.Context, old sql.Row, new sql. return fmt.Errorf("Runtime error: expected non-nil inputs to WorkspaceTableUpdater.Update") } + if !wtu.modifiable { + return errors.New(fmt.Sprintf("%s table is not modifiable due to schema change", wtu.workspaceTableName)) + } + valid, isStaged := validateWorkspaceUpdate(old, new) if !valid { return errors.New("only update of column 'staged' is allowed") @@ -205,6 +217,10 @@ func (wtd *WorkspaceTableDeleter) StatementBegin(ctx *sql.Context) { } func (wtd *WorkspaceTableDeleter) Delete(c *sql.Context, row sql.Row) error { + if !wtd.modifiable { + return errors.New(fmt.Sprintf("%s table is not modifiable due to schema change", wtd.workspaceTableName)) + } + isStaged := isTrue(row[stagedColumnIdx]) if isStaged { return fmt.Errorf("cannot delete staged rows from workspace") @@ -313,11 +329,13 @@ func validateWorkspaceUpdate(old, new sql.Row) (valid, staged bool) { func (wt *WorkspaceTable) Deleter(_ *sql.Context) sql.RowDeleter { cols := wt.headSchema.GetAllCols().Size() modifier := WorkspaceTableModifier{ - tableName: wt.userTblName, - headSch: wt.headSchema, - schemaLen: cols, - ws: wt.ws, - head: wt.head, + tableName: wt.userTblName, + workspaceTableName: wt.Name(), + headSch: wt.headSchema, + schemaLen: cols, + ws: wt.ws, + head: wt.head, + modifiable: wt.modifiable, } return &WorkspaceTableDeleter{ @@ -328,11 +346,13 @@ func (wt *WorkspaceTable) Deleter(_ *sql.Context) sql.RowDeleter { func (wt *WorkspaceTable) Updater(_ *sql.Context) sql.RowUpdater { cols := wt.headSchema.GetAllCols().Size() modifier := WorkspaceTableModifier{ - tableName: wt.userTblName, - headSch: wt.headSchema, - schemaLen: cols, - ws: wt.ws, - head: wt.head, + tableName: wt.userTblName, + workspaceTableName: wt.Name(), + headSch: wt.headSchema, + schemaLen: cols, + ws: wt.ws, + head: wt.head, + modifiable: wt.modifiable, } return &WorkspaceTableUpdater{ @@ -407,6 +427,12 @@ func NewWorkspaceTable(ctx *sql.Context, workspaceTableName string, tableName do fromSch = toSch } + modifiable := false + if fromSch != nil && toSch != nil { + // TODO: be more intelligent about schema migrations. This is pretty strict, but it's also correct. + modifiable = schema.ColCollsAreEqual(fromSch.GetAllCols(), toSch.GetAllCols()) + } + sch := sql.NewPrimaryKeySchema(GetDoltWorkspaceBaseSqlSchema()) baseDoltSch, err := sqlutil.ToDoltSchema(ctx, head, tableName, sch, head, sql.Collation_Default) if err != nil { @@ -430,6 +456,7 @@ func NewWorkspaceTable(ctx *sql.Context, workspaceTableName string, tableName do stagedDeltas: stgDel, workingDeltas: wkDel, headSchema: fromSch, + modifiable: modifiable, }, nil } diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go index f64df5fac3..bb3aef47f3 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go @@ -20,6 +20,7 @@ import ( ) var DoltWorkspaceScriptTests = []queries.ScriptTest{ + { Name: "dolt_workspace_* empty table", SetUpScript: []string{ @@ -269,7 +270,6 @@ var DoltWorkspaceScriptTests = []queries.ScriptTest{ }, }, }, - { Name: "dolt_workspace_* keyless table", SetUpScript: []string{ @@ -371,6 +371,7 @@ var DoltWorkspaceScriptTests = []queries.ScriptTest{ */ }, }, + { Name: "dolt_workspace_* prevent illegal updates", SetUpScript: []string{ @@ -1013,4 +1014,44 @@ var DoltWorkspaceScriptTests = []queries.ScriptTest{ }, }, }, + { + Name: "dolt_workspace_* schema change partial staging unstaging", + SetUpScript: []string{ + "create table tbl (x int primary key, y int)", + "insert into tbl values (42,42)", + "insert into tbl values (23,23)", + "call dolt_commit('-Am', 'creating table tbl')", + "alter table tbl add column why varchar(16)", + "update tbl set why = 'fourty two' where x = 42", + "update tbl set why = 'twenty three' where x = 23", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "select * from dolt_workspace_tbl", + Expected: []sql.Row{ + {0, false, "modified", 23, 23, "twenty three", 23, 23}, + {1, false, "modified", 42, 42, "fourty two", 42, 42}, + }, + }, + { + // TODO: This seems reasonable, but is difficult with current code structure. We'd need to intercept the + // query earlier to determine that the user just wants to reset the table. + Query: "delete from dolt_workspace_tbl", + ExpectedErrStr: "dolt_workspace_tbl table is not modifiable due to schema change", + }, + { + // TODO: We could support this... see above. This is basically a `dolt add tbl` + Query: "update dolt_workspace_tbl set staged = true", + ExpectedErrStr: "dolt_workspace_tbl table is not modifiable due to schema change", + }, + { + Query: "delete from dolt_workspace_tbl where id = 0", + ExpectedErrStr: "dolt_workspace_tbl table is not modifiable due to schema change", + }, + { + Query: "update dolt_workspace_tbl set staged = true where id = 0", + ExpectedErrStr: "dolt_workspace_tbl table is not modifiable due to schema change", + }, + }, + }, } From 2a73cd661122eda2af077a5645addba477e3bf0c Mon Sep 17 00:00:00 2001 From: Neil Macneale IV <46170177+macneale4@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:09:11 -0800 Subject: [PATCH 2/4] Update go/libraries/doltcore/sqle/dtables/workspace_table.go --- go/libraries/doltcore/sqle/dtables/workspace_table.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/libraries/doltcore/sqle/dtables/workspace_table.go b/go/libraries/doltcore/sqle/dtables/workspace_table.go index f4b52058f0..9ecba12d6d 100644 --- a/go/libraries/doltcore/sqle/dtables/workspace_table.go +++ b/go/libraries/doltcore/sqle/dtables/workspace_table.go @@ -64,7 +64,7 @@ type WorkspaceTable struct { headSchema schema.Schema // modifiable is true if the schemas before and after the update are identical. Used to reject updates that would - // be impossible perform - such as only stage one row when the entire schema of the table is being modified. + // be impossible to perform - such as only stage one row when the entire schema of the table is being modified. modifiable bool } From c18e931d7da2393bcc2bcafc87559ba0fc3df09b Mon Sep 17 00:00:00 2001 From: Neil Macneale IV <46170177+macneale4@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:10:11 -0800 Subject: [PATCH 3/4] Update go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go --- go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go index bb3aef47f3..decf1f19ef 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go @@ -20,7 +20,6 @@ import ( ) var DoltWorkspaceScriptTests = []queries.ScriptTest{ - { Name: "dolt_workspace_* empty table", SetUpScript: []string{ From c210a9f8dc0b5c6d0432957fa4541c9e4d773394 Mon Sep 17 00:00:00 2001 From: Neil Macneale IV <46170177+macneale4@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:10:31 -0800 Subject: [PATCH 4/4] Update go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go --- go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go index decf1f19ef..23e39e30aa 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_workspace.go @@ -370,7 +370,6 @@ var DoltWorkspaceScriptTests = []queries.ScriptTest{ */ }, }, - { Name: "dolt_workspace_* prevent illegal updates", SetUpScript: []string{