-
Notifications
You must be signed in to change notification settings - Fork 39
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
ensure consistent index ordering #83
Conversation
fixes gnormal#82 The databases return index data in a consistent order. The previous process did not respect that through its use of a map as an intermediate store. By using slices, we now guarantee that the template will see the indexes in the order the database returned them.
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.
looks good with a couple tiny tweaks if you could.
database/drivers/mysql/parse.go
Outdated
indexes[s.TableSchema] = schemaIndex | ||
} | ||
|
||
tableIndex, ok := schemaIndex[s.TableName] | ||
if !ok { | ||
tableIndex = make(map[string][]*database.Column) | ||
tableIndex = make([]*database.Index, 0) |
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.
you can drop the ,0 here, since that's the default (and I think go vet or go lint will complain at you)
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.
I had removed the 0 and go vet
complained, but I didn't spend too much time on it. I'll take another look.
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.
I have verified that the length parameter is required. The exception being []*database.Index{}
syntax.
database/drivers/mysql/parse.go
Outdated
@@ -126,16 +126,29 @@ func parse(log *log.Logger, conn string, schemaNames []string, filterTables func | |||
|
|||
schemaIndex, ok := indexes[s.TableSchema] | |||
if !ok { | |||
schemaIndex = make(map[string]map[string][]*database.Column) | |||
schemaIndex = make(map[string][]*database.Index) | |||
indexes[s.TableSchema] = schemaIndex | |||
} | |||
|
|||
tableIndex, ok := schemaIndex[s.TableName] |
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.
can you make this "tableIndexes" (or indices)? I think it would be more clear.
database/drivers/postgres/parse.go
Outdated
indexes[r.SchemaName] = schemaIndex | ||
} | ||
|
||
tableIndex, ok := schemaIndex[r.TableName] | ||
if !ok { | ||
tableIndex = make(map[string][]*database.Column) | ||
tableIndex = make([]*database.Index, 0) |
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.
same as above
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.
Hey sorry, got a chance to look at this again, and realized we can cut out those lines entirely. Thanks :)
database/drivers/mysql/parse.go
Outdated
tableIndex = make(map[string][]*database.Column) | ||
schemaIndex[s.TableName] = tableIndex | ||
tableIndices = make([]*database.Index, 0) | ||
schemaIndex[s.TableName] = tableIndices | ||
} |
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.
You can actually remove this entire if statement (and the ok above). The map will return a nil array if it doesn't exist, which is fine both for the range and the append below.
database/drivers/postgres/parse.go
Outdated
tableIndex = make(map[string][]*database.Column) | ||
schemaIndex[r.TableName] = tableIndex | ||
tableIndices = make([]*database.Index, 0) | ||
schemaIndex[r.TableName] = tableIndices | ||
} |
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.
same as above
fixes #82
The databases return index data in a consistent order. The previous
process did not respect that through its use of a map as an
intermediate store. By using slices, we now guarantee that the template
will see the indexes in the order the database returned them.