Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix update <table> set <column> = default #2165

Merged
merged 5 commits into from
Nov 28, 2023
Merged

fix update <table> set <column> = default #2165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
752c233
fix update set col = default
Nov 27, 2023
e87d967
adding tests
Nov 27, 2023
0fc5ef6
fix and tests for update with null default
Nov 27, 2023
0a9af02
remove todos
Nov 27, 2023
95badb6
fixing comment and removing onupdate
Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions enginetest/queries/script_queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4269,6 +4269,70 @@ CREATE TABLE tab3 (
},
},
},
{
Name: "update columns with default",
SetUpScript: []string{
"create table t (i int default 10, j varchar(128) default (concat('abc', 'def')));",
"insert into t values (100, 'a'), (200, 'b');",
"create table t2 (i int);",
"insert into t2 values (1), (2), (3);",
},
Assertions: []ScriptTestAssertion{
{
Query: "update t set i = default where i = 100;",
Expected: []sql.Row{
{types.OkResult{RowsAffected: 1, Info: plan.UpdateInfo{Matched: 1, Updated: 1}}},
},
},
{
Query: "select * from t order by i",
Expected: []sql.Row{
{10, "a"},
{200, "b"},
},
},
{
Query: "update t set j = default where i = 200;",
Expected: []sql.Row{
{types.OkResult{RowsAffected: 1, Info: plan.UpdateInfo{Matched: 1, Updated: 1}}},
},
},
{
Query: "select * from t order by i",
Expected: []sql.Row{
{10, "a"},
{200, "abcdef"},
},
},
{
Query: "update t set i = default, j = default;",
Expected: []sql.Row{
{types.OkResult{RowsAffected: 2, Info: plan.UpdateInfo{Matched: 2, Updated: 2}}},
},
},
{
Query: "select * from t order by i",
Expected: []sql.Row{
{10, "abcdef"},
{10, "abcdef"},
},
},
{
Query: "update t2 set i = default",
Expected: []sql.Row{
{types.OkResult{RowsAffected: 3, Info: plan.UpdateInfo{Matched: 3, Updated: 3}}},
},
},
{
Query: "select * from t2",
Expected: []sql.Row{
{nil},
{nil},
{nil},
},
},
},
},
}

var SpatialScriptTests = []ScriptTest{
Expand Down
13 changes: 10 additions & 3 deletions sql/planbuilder/dml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,23 +220,30 @@ func (b *Builder) assignmentExprsToExpressions(inScope *scope, e ast.AssignmentE
startWinCnt = len(inScope.windowFuncs)
}

tableSch := inScope.node.Schema()
tableSch := b.resolveSchemaDefaults(inScope, inScope.node.Schema())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing this here again smells weird. It doesn't look like DML nodes implement TargetSchema, but should they implement something similar and do this sooner? Or do we really not want the node schemas to resolve defaults for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema defaults for updates were unresolved, so we definitely need to call this somewhere.
It doesn't seem like Update nodes need TargetSchema, as long as the update expressions are resolved and correct.


for i, updateExpr := range e {
colName := b.buildScalar(inScope, updateExpr.Name)

// Prevent update of generated columns
innerExpr := b.buildScalar(inScope, updateExpr.Expr)
if gf, ok := colName.(*expression.GetField); ok {
colIdx := tableSch.IndexOfColName(gf.Name())
// TODO: during trigger parsing the table in the node is unresolved, so we need this additional bounds check
// This means that trigger execution will be able to update generated columns
// Prevent update of generated columns
if colIdx >= 0 && tableSch[colIdx].Generated != nil {
err := sql.ErrGeneratedColumnValue.New(tableSch[colIdx].Name, inScope.node.(sql.NameableNode).Name())
b.handleErr(err)
}

// Replace default with column default from resolved schema
if _, ok := updateExpr.Expr.(*ast.Default); ok {
if colIdx >= 0 {
innerExpr = expression.WrapExpression(tableSch[colIdx].Default)
}
}
}

innerExpr := b.buildScalar(inScope, updateExpr.Expr)
updateExprs[i] = expression.NewSetField(colName, innerExpr)
if inScope.groupBy != nil {
if len(inScope.groupBy.aggs) > startAggCnt {
Expand Down