Skip to content

Fix inspector column types #661

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 16, 2018
Merged

Fix inspector column types #661

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dfc8aa2
Ignore not available columns when applying column types
esnunes Nov 1, 2018
5f15f8a
Remove not needed call by grouping columnLists
esnunes Nov 1, 2018
7e93451
Merge branch 'master' into fix-inspector-column-types
esnunes Nov 5, 2018
ebba4f4
Merge branch 'master' into fix-inspector-column-types
Nov 13, 2018
736696f
Merge branch 'master' into fix-inspector-column-types
Nov 29, 2018
179a3c2
Merge branch 'master' into fix-inspector-column-types
Dec 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 23 additions & 33 deletions go/logic/inspect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,7 @@ func (this *Inspector) inspectOriginalAndGhostTables() (err error) {
// This additional step looks at which columns are unsigned. We could have merged this within
// the `getTableColumns()` function, but it's a later patch and introduces some complexity; I feel
// comfortable in doing this as a separate step.
this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, this.migrationContext.OriginalTableColumns, this.migrationContext.SharedColumns)
this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, &this.migrationContext.UniqueKey.Columns)
this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, this.migrationContext.OriginalTableColumns, this.migrationContext.SharedColumns, &this.migrationContext.UniqueKey.Columns)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a small improvement, instead of calling applyColumnTypes twice for the same databaseName and tableName, I've moved the &this.migrationContext.UniqueKey.Columns to this call.

this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.GetGhostTableName(), this.migrationContext.GhostTableColumns, this.migrationContext.MappedSharedColumns)

for i := range this.migrationContext.SharedColumns.Columns() {
Expand Down Expand Up @@ -552,44 +551,35 @@ func (this *Inspector) applyColumnTypes(databaseName, tableName string, columnsL
err := sqlutils.QueryRowsMap(this.db, query, func(m sqlutils.RowMap) error {
columnName := m.GetString("COLUMN_NAME")
columnType := m.GetString("COLUMN_TYPE")
if strings.Contains(columnType, "unsigned") {
for _, columnsList := range columnsLists {
columnsList.SetUnsigned(columnName)
for _, columnsList := range columnsLists {
column := columnsList.GetColumn(columnName)
if column == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The for loop was changed in order to prevent accessing a column that is not available in the columnsList object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code ignores columns that are not available in the columnsList and continues iterating over the next columns.

continue
}
}
if strings.Contains(columnType, "mediumint") {
for _, columnsList := range columnsLists {
columnsList.GetColumn(columnName).Type = sql.MediumIntColumnType
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code fails when columnsList.GetColumn(columnName) returns nil. This is going to happen in this.migrationContext.MappedSharedColumns and this.migrationContext.SharedColumns due to the fact they they only have the intersection of original and ghost tables. If you add a new column by the end of the column list this problem won't be noticed as the code is going to fail when setting the type of the new column (which is not used during copy of data), but if you alter table and add a column in the second position, all other columns will have the default Type (0) because the code will not iterate over all of them.


if strings.Contains(columnType, "unsigned") {
column.IsUnsigned = true
}
}
if strings.Contains(columnType, "timestamp") {
for _, columnsList := range columnsLists {
columnsList.GetColumn(columnName).Type = sql.TimestampColumnType
if strings.Contains(columnType, "mediumint") {
column.Type = sql.MediumIntColumnType
}
}
if strings.Contains(columnType, "datetime") {
for _, columnsList := range columnsLists {
columnsList.GetColumn(columnName).Type = sql.DateTimeColumnType
if strings.Contains(columnType, "timestamp") {
column.Type = sql.TimestampColumnType
}
}
if strings.Contains(columnType, "json") {
for _, columnsList := range columnsLists {
columnsList.GetColumn(columnName).Type = sql.JSONColumnType
if strings.Contains(columnType, "datetime") {
column.Type = sql.DateTimeColumnType
}
}
if strings.Contains(columnType, "float") {
for _, columnsList := range columnsLists {
columnsList.GetColumn(columnName).Type = sql.FloatColumnType
if strings.Contains(columnType, "json") {
column.Type = sql.JSONColumnType
}
}
if strings.HasPrefix(columnType, "enum") {
for _, columnsList := range columnsLists {
columnsList.GetColumn(columnName).Type = sql.EnumColumnType
if strings.Contains(columnType, "float") {
column.Type = sql.FloatColumnType
}
}
if charset := m.GetString("CHARACTER_SET_NAME"); charset != "" {
for _, columnsList := range columnsLists {
columnsList.SetCharset(columnName, charset)
if strings.HasPrefix(columnType, "enum") {
column.Type = sql.EnumColumnType
}
if charset := m.GetString("CHARACTER_SET_NAME"); charset != "" {
column.Charset = charset
}
}
return nil
Expand Down