GMS tests have to resolve defaults #1952
Conversation
|
Can we add tests to GMS for this? It's not immediately clear to me what the purpose of this change is, or what bugs it fixes. Tests would illustrate that. |
| newDef, _, _ := transform.Expr(cCopy.Default, func(e sql.Expression) (sql.Expression, transform.TreeIdentity, error) { | ||
| switch e := e.(type) { | ||
| case *expression.GetField: | ||
| return expression.NewGetField(e.Index(), e.Type(), e.Name(), e.IsNullable()), transform.NewTree, nil |
There was a problem hiding this comment.
What is the purpose of this line? It looks like it just creates a new getField identical to the old one.
There was a problem hiding this comment.
We have to serialize defaults without the table qualifier. I will add a comment
None of our column default expression tests were tested on the GMS side. So load data, alter table, show queries, insert/triggers, etc. This change makes all of our memory tests actually test resolving column defaults. |
|
For a bit more context, the lifecycle of column defaults go through a serialization phase into and off disk. We write expression strings that represent default expressions that have to be individually parsed, resolved, and indexed. Managing this lifecycle is an additional hard step in many ALTER and DML statements. The GMS memory implementation was lazy and never serialized column defaults, making the GMS tests of column defaults trivially easy to pass. The Dolt implementation was our only layer of defaults testing. This PR forces the GMS memory tables to act as if they serialize column defaults, forcing us to test column defaults more heavily. The result was that several GMS-only tests broke, which were also gaps in Dolt's testing and correctness. |
Force GMS to resolve column defaults, fix bugs exposed by additional testing. Dolt enginetests pass locally.