-
-
Notifications
You must be signed in to change notification settings - Fork 209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix in-memory implementation of RenameTable to read from session #2398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find, but the wrong fix. Needs more digging.
enginetest/queries/script_queries.go
Outdated
@@ -6182,6 +6182,34 @@ where | |||
}, | |||
}, | |||
}, | |||
{ | |||
Name: "alter statements apply correctly", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of a vague name, what specifically are you testing here? How is this not covered in TestAlterTable e.g.?
memory/table.go
Outdated
@@ -1415,6 +1415,7 @@ func (t *Table) ModifyColumn(ctx *sql.Context, columnName string, column *sql.Co | |||
} | |||
} | |||
|
|||
t.db.tables[t.name] = t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to happen when the session is committed. Why isn't it? You have to fix that bug, not mess up the global state here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The in-memory implementation of RenameTable uses data from the BaseDatabase, instead of reading it from the session.
This is problematic when there are multiple alter statements.
Additonally, includes some small refactor so all functions are pointer receiver instead of a mix.
fixes #2396