sqlfmt: omit generated columns from diff -r sql - #11476
Conversation
dolt diff -r sql was emitting STORED generated columns in INSERT and UPDATE statements, which the engine rejects on replay. Populate Column.Generated and Virtual in schemaFromCreateTableStmt so IsGenerated works on the CLI SQL-diff path, and skip generated columns in SqlRowAsUpdateStmt / GenerateDataDiffStatement (mirroring the existing INSERT filters). Fixes dolthub#11445
|
@vishnujayvel Could you shorten the description to be similar to our teams' release notes? They come up as release notes and this change is relatively small. |
|
Done — shortened the description to match the team's release-note style. Thanks again for the repro in #11445 — it named both failure points, which is why this landed as a small diff. I've parked the long-form root cause and test plan below so it stays on the PR for review without ending up in the release notes. Original description (root cause + test plan)
The same emission path backs the Fixes #11445 Thanks to @elianddb (Elian) for the clear repro and for tracing both failure GotUPDATE `t` SET `a`=99,`g`=100 WHERE `id`=1;
INSERT INTO `t` (`id`,`a`,`g`) VALUES (2,20,21);Want / after this changeUPDATE `t` SET `a`=99 WHERE `id`=1;
INSERT INTO `t` (`id`,`a`) VALUES (2,20);Root cause (two parts)
Fix
Test plan
Notes
|
elianddb
left a comment
There was a problem hiding this comment.
A couple of requested changes to rm extra code, and uncovered edge case.
| // | ||
| // TODO(elianddb): Schema isn't recording column's Generated marker | ||
| // correctly, so Column.IsGenerated doesn't filter. | ||
| // Generated columns are skipped, matching InsertStatementPrefix and |
There was a problem hiding this comment.
No need to mention the other shared helpers, only the the Generated Columns behavior. Would also move to second paragraph.
| if col.IsGenerated() { | ||
| continue | ||
| } | ||
| updatedCols.Add(col.Name) |
There was a problem hiding this comment.
Remove no need to duplicate filtering logic here.
| err := tableSch.GetAllCols().Iter(func(_ uint64, col schema.Column) (stop bool, err error) { | ||
| if colsToUpdate.Contains(col.Name) { | ||
| if colsToUpdate.Contains(col.Name) && !col.IsGenerated() { | ||
| if seenOne { |
There was a problem hiding this comment.
There's an empty list edge case here !seenOne add an assertion to existing test.
| stmt, err := sqlfmt.SqlRowAsUpdateStmt(sql.NewEmptyContext(), sql.Row{int64(1), "x", "x!"}, "table_name", sch, colsToUpdate) | ||
|
|
||
| require.NoError(t, err) | ||
| assert.Equal(t, "UPDATE `table_name` SET `a`='x' WHERE `id`=1;", stmt) |
There was a problem hiding this comment.
assertion for empty string
dolt diff -r sqlno longer assigns generated columns in theINSERTandUPDATEstatements it emits, so its output can be replayed. Previously the engine rejected those assignments; where the diff is emitted asDELETE+INSERT— keyless tables, and tables whose primary key is itself generated — replay deleted the old row and then failed to reinsert it, silently losing the row.dolt_patch()'sstatementoutput is fixed by the same change.schemaFromCreateTableStmtnow populatesGenerated,Virtual, andOnUpdateonschema.Column, soIsGenerated()is correct for schemas built fromSHOW CREATE TABLE.SqlRowAsUpdateStmtskips generated columns, andGenerateDataDiffStatementdrops them fromcolsToUpdate.Fix #11445
Close #11699