Bug fix: Call StatementBegin for all TableEditors used in insertIter
#2892
Conversation
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
Maybe Max has a comment on this one too, seems a bit sus.
| if updater != nil { | ||
| eds = append(eds, updater) | ||
| } | ||
| return plan.NewTableEditorIter(insertIter, eds...), nil |
There was a problem hiding this comment.
AFAICT we never use more than one editor in practice, despite this constructor accepting multiple editors. I guess if it works and the tests pass it's fine?
But it seems like there should be at least one engine test that doesn't work before this change and passes now.
There was a problem hiding this comment.
insertIter takes a replacer, an updater, and an inserter. If the replacer isn't set, it'll use the inserter, and if the inserter fails, it'll handle an ON DUPLICATE UPDATE clause and use the updater.
This doesn't repro with a regular, user table, but is triggered by the docWriter implementation (specifically, the one held in the updater field), because it needs StatementBegin() called to initialize its table writer. Without this, there's a panic from trying to use the nil table writer.
max-hoffman
left a comment
max-hoffman
left a comment
There was a problem hiding this comment.
The change looks good to me, the editor interfaces are very indirect and different between Dolt and GMS if i'm remembering correctly so good that there's an added test.
insertItercan be given asql.RowInserterand asql.RowUpdater, and when theON DUPLICATE UPDATEclause is present, it can use both to update a table.StatementBeginwas only being called on thesql.RowInserterand not for thesql.RowUpdater, which caused a problem for some implementations (e.g.docsWriter) that depend onStatementBeginbeing called.Test for this added to the Dolt package (dolthub/dolt#8988), using the
docWriterimplementation where this bug was originally discovered.