Add transactions test #497
Conversation
enginetest/transaction_queries.go
Outdated
| }, | ||
| { | ||
| Query: "/* client a */ select * from t2 order by x", | ||
| Expected: []sql.Row{{10, 10}, {12, 12}}, // TODO: Why does it not have (11, 11) |
There was a problem hiding this comment.
@zachmu Can you sanity check this with me? Isn't this a bug?
sql/expression/auto_increment.go
Outdated
| val, err := i.autoIncVal.Eval(ctx, row) | ||
| if err != nil { | ||
| return nil, err | ||
| return given, nil |
There was a problem hiding this comment.
In general, Eval should be safe to call multiple times, and this change makes that not the case
I think it's fine for now, and you have tests, but add a note for whoever breaks this
sql/core.go
Outdated
| // PeekNextAutoIncrementValue returns the expected next AUTO_INCREMENT value but does not require | ||
| // implementations to update their state. | ||
| PeekNextAutoIncrementValue(*Context) (interface{}, error) | ||
| // GetNextAutoIncrementValue gets the next AUTO_INCREMENT value. |
There was a problem hiding this comment.
Need to explain the semantics of the added param
sql/expression/auto_increment.go
Outdated
| } | ||
|
|
||
| nextVal, err := NewIncrement(i.autoIncVal).Eval(ctx, row) | ||
| // Integrator answer | ||
| // TODO: Calling Eval in general should be safe but since this gets called per row this should be fine. |
There was a problem hiding this comment.
Comment needs more explanation than this
The problem is that if we call Eval on this expression more than once, we will end up skipping keys
sql/expression/auto_increment.go
Outdated
| nextVal, err := NewIncrement(i.autoIncVal).Eval(ctx, row) | ||
| // Integrator answer | ||
| // TODO: Calling Eval in general should be safe but since this gets called per row this should be fine. | ||
| last, err := i.autoTbl.GetNextAutoIncrementValue(ctx, given) |
| val, err := i.autoIncVal.Eval(ctx, row) | ||
| if err != nil { | ||
| return nil, err | ||
| if cmp == 0 { |
There was a problem hiding this comment.
Can't you check if it's nil directly rather than call compare above?
There was a problem hiding this comment.
0 is treated as NULL for auto increment columns. I'll fix.
Adds tests for transactions and auto increment values.