fix update <table> set <column> = default
#2165
Conversation
sql/planbuilder/dml.go
Outdated
| if colIdx >= 0 && tableSch[colIdx].Generated != nil { | ||
| err := sql.ErrGeneratedColumnValue.New(tableSch[colIdx].Name, inScope.node.(sql.NameableNode).Name()) | ||
| b.handleErr(err) | ||
| } | ||
| // Replace update expressions with default |
There was a problem hiding this comment.
the ordering/nesting here seems weird, can we resolve the default during buildScalar? or separately from this if block? or does it really depend on colName?
There was a problem hiding this comment.
I just reused logic from InsertDestination. We seem to treat ast.Default as a placeholder to be replace with whatever is stored in the schema.
BuildScalar would need the resolved schema
I could separate this into its own block, but it would look the same as the block it's already in
sql/planbuilder/ddl.go
Outdated
| @@ -1032,6 +1032,7 @@ func (b *Builder) tableSpecToSchema(inScope, outScope *scope, db sql.Database, t | |||
|
|
|||
| defaults := make([]ast.Expr, len(tableSpec.Columns)) | |||
| generated := make([]ast.Expr, len(tableSpec.Columns)) | |||
| updates := make([]ast.Expr, len(tableSpec.Columns)) | |||
There was a problem hiding this comment.
did we only need this for update default? should we have a separate test for this if there is a non-default query that errors without it?
There was a problem hiding this comment.
Oops this was leftover from an attempt to implement OnUpdate functionality; will remove
| @@ -220,23 +220,29 @@ func (b *Builder) assignmentExprsToExpressions(inScope *scope, e ast.AssignmentE | |||
| startWinCnt = len(inScope.windowFuncs) | |||
| } | |||
|
|
|||
| tableSch := inScope.node.Schema() | |||
| tableSch := b.resolveSchemaDefaults(inScope, inScope.node.Schema()) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This PR fixes a bug where attempting to update a column to its default would throw an unresolved error.