Skip to content

Commit b1bb38e

Browse files
fulghumfulghum
committed
Fixing gaps in logic for testing if TargetSchema plan nodes are resolved.
Fixes: dolthub/dolt#6206
1 parent f20ba79 commit b1bb38e

File tree

10 files changed

+79
-88
lines changed

10 files changed

+79
-88
lines changed

‎enginetest/enginetests.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5683,6 +5683,36 @@ func TestAlterTable(t *testing.T, harness Harness) {
56835683

56845684
TestQueryWithContext(t, ctx, e, harness, "SELECT * FROM t40", []sql.Row{{1, 1}}, nil, nil)
56855685
})
5686+
5687+
TestScript(t, harness, queries.ScriptTest{
5688+
// https://github.com/dolthub/dolt/issues/6206
5689+
Name: "alter table containing column default value expressions",
5690+
SetUpScript: []string{
5691+
"create table t (pk int primary key, col1 timestamp default current_timestamp(), col2 varchar(1000), index idx1 (pk, col1));",
5692+
},
5693+
Assertions: []queries.ScriptTestAssertion{
5694+
{
5695+
Query: "alter table t alter column col2 DROP DEFAULT;",
5696+
Expected: []sql.Row{},
5697+
},
5698+
{
5699+
Query: "show create table t;",
5700+
Expected: []sql.Row{{"t", "CREATE TABLE `t` (\n `pk` int NOT NULL,\n `col1` timestamp(6) DEFAULT (CURRENT_TIMESTAMP()),\n `col2` varchar(1000),\n PRIMARY KEY (`pk`),\n KEY `idx1` (`pk`,`col1`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}},
5701+
},
5702+
{
5703+
Query: "alter table t alter column col2 SET DEFAULT 'FOO!';",
5704+
Expected: []sql.Row{},
5705+
},
5706+
{
5707+
Query: "show create table t;",
5708+
Expected: []sql.Row{{"t", "CREATE TABLE `t` (\n `pk` int NOT NULL,\n `col1` timestamp(6) DEFAULT (CURRENT_TIMESTAMP()),\n `col2` varchar(1000) DEFAULT 'FOO!',\n PRIMARY KEY (`pk`),\n KEY `idx1` (`pk`,`col1`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}},
5709+
},
5710+
{
5711+
Query: "alter table t drop index idx1;",
5712+
Expected: []sql.Row{{types.NewOkResult(0)}},
5713+
},
5714+
},
5715+
})
56865716
}
56875717

56885718
func NewColumnDefaultValue(expr sql.Expression, outType sql.Type, representsLiteral, isParenthesized, mayReturnNil bool) *sql.ColumnDefaultValue {

‎sql/analyzer/resolve_column_defaults.go‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -173,19 +173,33 @@ func resolveColumnDefaults(ctx *sql.Context, _ *Analyzer, n sql.Node, _ *plan.Sc
173173
col := sch[index]
174174

175175
eWrapper := expression.WrapExpression(node.Default)
176-
newExpr, same, err := resolveColumnDefault(ctx, col, eWrapper)
176+
newExpr, sameColumnDefault, err := resolveColumnDefault(ctx, col, eWrapper)
177177
if err != nil {
178178
return node, transform.SameTree, err
179179
}
180-
if same {
181-
return node, transform.SameTree, nil
182-
}
183180

184181
newNode, err := node.WithDefault(newExpr)
185182
if err != nil {
186183
return node, transform.SameTree, err
187184
}
188-
return newNode, transform.NewTree, nil
185+
186+
// After resolving the new column default value, ensure all column defaults in the target schema are resolved
187+
newNode, sameTargetSchema, err := transform.OneNodeExpressions(newNode, func(e sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
188+
eWrapper, ok := e.(*expression.Wrapper)
189+
if !ok {
190+
return e, transform.SameTree, nil
191+
}
192+
193+
col, err := lookupColumnForTargetSchema(ctx, node, colIndex)
194+
if err != nil {
195+
return nil, transform.SameTree, err
196+
}
197+
colIndex++
198+
199+
return resolveColumnDefault(ctx, col, eWrapper)
200+
})
201+
202+
return newNode, sameColumnDefault && sameTargetSchema, err
189203
case sql.SchemaTarget:
190204
return transform.OneNodeExpressions(n, func(e sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
191205
eWrapper, ok := e.(*expression.Wrapper)
@@ -338,7 +352,7 @@ func lookupColumnForTargetSchema(_ *sql.Context, node sql.SchemaTarget, colIndex
338352
if index == -1 {
339353
return nil, sql.ErrTableColumnNotFound.New(n2.Table, n2.ColumnName)
340354
}
341-
return n2.Schema()[index], nil
355+
return schema[index], nil
342356
default:
343357
if colIndex < len(schema) {
344358
return schema[colIndex], nil

‎sql/plan/alter_default.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,11 @@ func (d *AlterDefaultSet) String() string {
6464
return fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET DEFAULT %s", d.Table.String(), d.ColumnName, d.Default.String())
6565
}
6666

67+
// Resolved implements the sql.Node interface.
68+
func (d *AlterDefaultDrop) Resolved() bool {
69+
return d.ddlNode.Resolved() && d.Table.Resolved() && d.targetSchema.Resolved()
70+
}
71+
6772
// RowIter implements the sql.Node interface.
6873
func (d *AlterDefaultSet) RowIter(ctx *sql.Context, row sql.Row) (sql.RowIter, error) {
6974
// Grab the table fresh from the database.
@@ -123,7 +128,7 @@ func (d *AlterDefaultSet) CollationCoercibility(ctx *sql.Context) (collation sql
123128

124129
// Resolved implements the sql.Node interface.
125130
func (d *AlterDefaultSet) Resolved() bool {
126-
return d.Table.Resolved() && d.ddlNode.Resolved() && d.Default.Resolved()
131+
return d.ddlNode.Resolved() && d.Table.Resolved() && d.Default.Resolved() && d.targetSchema.Resolved()
127132
}
128133

129134
func (d *AlterDefaultSet) Expressions() []sql.Expression {

‎sql/plan/alter_index.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ func (p AlterIndex) String() string {
257257
}
258258

259259
func (p *AlterIndex) Resolved() bool {
260-
return p.Table.Resolved() && p.ddlNode.Resolved()
260+
return p.Table.Resolved() && p.ddlNode.Resolved() && p.targetSchema.Resolved()
261261
}
262262

263263
// Children implements the sql.Node interface.

‎sql/plan/alter_pk.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,7 @@ func NewAlterDropPk(db sql.Database, table sql.Node) *AlterPK {
6767
}
6868

6969
func (a *AlterPK) Resolved() bool {
70-
for _, expr := range a.Expressions() {
71-
if expr.Resolved() == false {
72-
return false
73-
}
74-
}
75-
76-
return a.Table.Resolved() && a.ddlNode.Resolved()
70+
return a.Table.Resolved() && a.ddlNode.Resolved() && a.targetSchema.Resolved()
7771
}
7872

7973
func (a *AlterPK) String() string {

‎sql/plan/alter_table.go‎

Lines changed: 4 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -303,17 +303,7 @@ func (a AddColumn) WithExpressions(exprs ...sql.Expression) (sql.Node, error) {
303303

304304
// Resolved implements the Resolvable interface.
305305
func (a *AddColumn) Resolved() bool {
306-
if !(a.ddlNode.Resolved() && a.Table.Resolved() && a.column.Default.Resolved()) {
307-
return false
308-
}
309-
310-
for _, col := range a.targetSch {
311-
if !col.Default.Resolved() {
312-
return false
313-
}
314-
}
315-
316-
return true
306+
return a.ddlNode.Resolved() && a.Table.Resolved() && a.column.Default.Resolved() && a.targetSch.Resolved()
317307
}
318308

319309
// WithTargetSchema implements sql.SchemaTarget
@@ -544,17 +534,7 @@ func (d *DropColumn) Schema() sql.Schema {
544534
}
545535

546536
func (d *DropColumn) Resolved() bool {
547-
if !d.Table.Resolved() && !d.ddlNode.Resolved() {
548-
return false
549-
}
550-
551-
for _, col := range d.targetSchema {
552-
if !col.Default.Resolved() {
553-
return false
554-
}
555-
}
556-
557-
return true
537+
return d.Table.Resolved() && d.ddlNode.Resolved() && d.targetSchema.Resolved()
558538
}
559539

560540
func (d *DropColumn) Children() []sql.Node {
@@ -667,17 +647,7 @@ func (r *RenameColumn) DebugString() string {
667647
}
668648

669649
func (r *RenameColumn) Resolved() bool {
670-
if !r.Table.Resolved() && r.ddlNode.Resolved() {
671-
return false
672-
}
673-
674-
for _, col := range r.targetSchema {
675-
if !col.Default.Resolved() {
676-
return false
677-
}
678-
}
679-
680-
return true
650+
return r.Table.Resolved() && r.ddlNode.Resolved() && r.targetSchema.Resolved()
681651
}
682652

683653
func (r *RenameColumn) Schema() sql.Schema {
@@ -838,17 +808,7 @@ func (m ModifyColumn) WithExpressions(exprs ...sql.Expression) (sql.Node, error)
838808

839809
// Resolved implements the Resolvable interface.
840810
func (m *ModifyColumn) Resolved() bool {
841-
if !(m.Table.Resolved() && m.column.Default.Resolved() && m.ddlNode.Resolved()) {
842-
return false
843-
}
844-
845-
for _, col := range m.targetSchema {
846-
if !col.Default.Resolved() {
847-
return false
848-
}
849-
}
850-
851-
return true
811+
return m.Table.Resolved() && m.column.Default.Resolved() && m.ddlNode.Resolved() && m.targetSchema.Resolved()
852812
}
853813

854814
func (m *ModifyColumn) ValidateDefaultPosition(tblSch sql.Schema) error {

‎sql/plan/ddl.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -223,16 +223,10 @@ func (c *CreateTable) PkSchema() sql.PrimaryKeySchema {
223223

224224
// Resolved implements the Resolvable interface.
225225
func (c *CreateTable) Resolved() bool {
226-
if !c.ddlNode.Resolved() {
226+
if !c.ddlNode.Resolved() || !c.CreateSchema.Schema.Resolved() {
227227
return false
228228
}
229229

230-
for _, col := range c.CreateSchema.Schema {
231-
if !col.Default.Resolved() {
232-
return false
233-
}
234-
}
235-
236230
for _, chDef := range c.ChDefs {
237231
if !chDef.Expr.Resolved() {
238232
return false

‎sql/plan/show_create_table.go‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -59,17 +59,7 @@ func NewShowCreateTableWithAsOf(table sql.Node, isView bool, asOf sql.Expression
5959

6060
// Resolved implements the Resolvable interface.
6161
func (sc *ShowCreateTable) Resolved() bool {
62-
if !sc.Child.Resolved() {
63-
return false
64-
}
65-
66-
for _, col := range sc.targetSchema {
67-
if !col.Default.Resolved() {
68-
return false
69-
}
70-
}
71-
72-
return true
62+
return sc.Child.Resolved() && sc.targetSchema.Resolved()
7363
}
7464

7565
func (sc ShowCreateTable) WithChildren(children ...sql.Node) (sql.Node, error) {

‎sql/plan/showcolumns.go‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -74,17 +74,7 @@ func (s *ShowColumns) Schema() sql.Schema {
7474

7575
// Resolved implements the sql.Node interface.
7676
func (s *ShowColumns) Resolved() bool {
77-
if !s.Child.Resolved() {
78-
return false
79-
}
80-
81-
for _, col := range s.targetSchema {
82-
if !col.Default.Resolved() {
83-
return false
84-
}
85-
}
86-
87-
return true
77+
return s.Child.Resolved() && s.targetSchema.Resolved()
8878
}
8979

9080
func (s *ShowColumns) Expressions() []sql.Expression {

‎sql/schemas.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,20 @@ func (s Schema) HasAutoIncrement() bool {
118118
return false
119119
}
120120

121+
// Resolved returns true if this schema is fully resolved. Currently, the only piece of a schema that needs
122+
// to be resolved are any column default value expressions.
123+
func (s Schema) Resolved() bool {
124+
for _, c := range s {
125+
if c.Default != nil {
126+
if !c.Default.Resolved() {
127+
return false
128+
}
129+
}
130+
}
131+
132+
return true
133+
}
134+
121135
func IsKeyless(s Schema) bool {
122136
for _, c := range s {
123137
if c.PrimaryKey {

0 commit comments

Comments
 (0)