Skip to content

Commit

Permalink
fixed few bugs and QOL improvements (#628)
Browse files Browse the repository at this point in the history 
* fixed some bugs

* addressing comments

* ng build files
  • Loading branch information
@shreyakhajanchi
shreyakhajanchi authored Sep 28, 2023
1 parent 287bec4 commit dd43f5b
Show file tree
Hide file tree
Showing 18 changed files with 125 additions and 100 deletions.
11 changes: 5 additions & 6 deletions internal/convert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ const (
DefaultValue SchemaIssue = iota
ForeignKey
MissingPrimaryKey
UniqueIndexPrimaryKey
MultiDimensionalArray
NoGoodType
Numeric
Expand Down Expand Up @@ -403,7 +404,7 @@ func (conv *Conv) AddPrimaryKeys() {
for _, indexKey := range index.Keys {
ct.PrimaryKeys = append(ct.PrimaryKeys, ddl.IndexKey{ColId: indexKey.ColId, Desc: indexKey.Desc, Order: indexKey.Order})
conv.UniquePKey[t] = append(conv.UniquePKey[t], indexKey.ColId)
addMissingPrimaryKeyWarning(ct.Id, indexKey.ColId, conv)
addMissingPrimaryKeyWarning(ct.Id, indexKey.ColId, conv, UniqueIndexPrimaryKey)
}
primaryKeyPopulated = true
ct.Indexes = append(ct.Indexes[:i], ct.Indexes[i+1:]...)
Expand All @@ -418,25 +419,23 @@ func (conv *Conv) AddPrimaryKeys() {
ct.ColDefs[columnId] = ddl.ColumnDef{Name: k, Id: columnId, T: ddl.Type{Name: ddl.String, Len: 50}}
ct.PrimaryKeys = []ddl.IndexKey{{ColId: columnId, Order: 1}}
conv.SyntheticPKeys[t] = SyntheticPKey{columnId, 0}
addMissingPrimaryKeyWarning(ct.Id, columnId, conv)
addMissingPrimaryKeyWarning(ct.Id, columnId, conv, MissingPrimaryKey)
}
conv.SpSchema[t] = ct
}
}
}

// Add 'Missing Primary Key' as a Warning inside ColumnLevelIssues of conv object
func addMissingPrimaryKeyWarning(tableId string, colId string, conv *Conv) {
func addMissingPrimaryKeyWarning(tableId string, colId string, conv *Conv, schemaIssue SchemaIssue) {
tableLevelIssues := conv.SchemaIssues[tableId].TableLevelIssues
var columnLevelIssues map[string][]SchemaIssue
if tableIssues, ok := conv.SchemaIssues[tableId]; ok {
columnLevelIssues = tableIssues.ColumnLevelIssues
} else {
columnLevelIssues = make(map[string][]SchemaIssue)
}
issues := columnLevelIssues[colId]
issues = append(issues, MissingPrimaryKey)
columnLevelIssues[colId] = issues
columnLevelIssues[colId] = append(columnLevelIssues[colId], schemaIssue)
conv.SchemaIssues[tableId] = TableIssues{
TableLevelIssues: tableLevelIssues,
ColumnLevelIssues: columnLevelIssues,
Expand Down
70 changes: 29 additions & 41 deletions internal/reports/report_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,29 +130,6 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}

}
if syntheticPK != nil {
// Warnings about synthetic primary keys must be handled as a special case
// because we have a Spanner column with no matching source DB col.
// Much of the generic code for processing issues assumes we have both.
if p.severity == warning {
toAppend := Issue{
Category: IssueDB[internal.MissingPrimaryKey].Category,
Description: fmt.Sprintf("Column '%s' was added because table '%s' didn't have a primary key. Spanner requires a primary key for every table", *syntheticPK, conv.SpSchema[tableId].Name),
}
l = append(l, toAppend)
}
}
if uniquePK != nil {
// Warning about using a column with unique constraint as primary key
// in case primary key is absent.
if p.severity == warning {
toAppend := Issue{
Category: IssueDB[internal.MissingPrimaryKey].Category,
Description: fmt.Sprintf("UNIQUE constraint on column(s) '%s' replaced with primary key since table '%s' didn't have one. Spanner requires a primary key for every table", strings.Join(uniquePK, ", "), conv.SpSchema[tableId].Name),
}
l = append(l, toAppend)
}
}

if p.severity == warning {
for _, spFk := range conv.SpSchema[tableId].ForeignKeys {
Expand Down Expand Up @@ -261,11 +238,11 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
case internal.Widened:
toAppend := Issue{
Category: IssueDB[i].Category,
Description: fmt.Sprintf("Table %s: %s e.g. for column '%s', source DB type %s is mapped to Spanner data type %s", conv.SpSchema[tableId].Name, IssueDB[i].Brief, spColName, srcColType, spColType),
Description: fmt.Sprintf("Table '%s': %s e.g. for column '%s', source DB type %s is mapped to Spanner data type %s", conv.SpSchema[tableId].Name, IssueDB[i].Brief, spColName, srcColType, spColType),
}
l = append(l, toAppend)
case internal.HotspotTimestamp:
str := fmt.Sprintf(" %s for Table %s and Column %s", IssueDB[i].Brief, spSchema.Name, spColName)
str := fmt.Sprintf(" %s for Table '%s' and Column '%s'", IssueDB[i].Brief, spSchema.Name, spColName)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -275,7 +252,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
l = append(l, toAppend)
}
case internal.HotspotAutoIncrement:
str := fmt.Sprintf(" %s for Table %s and Column %s", IssueDB[i].Brief, spSchema.Name, spColName)
str := fmt.Sprintf(" %s for Table '%s' and Column '%s'", IssueDB[i].Brief, spSchema.Name, spColName)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -286,7 +263,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
case internal.InterleavedNotInOrder:
parent, _, _ := getInterleaveDetail(conv, tableId, colId, i)
str := fmt.Sprintf(" Table %s can be interleaved with table %s %s %s and Column %s", spSchema.Name, parent, IssueDB[i].Brief, spSchema.Name, spColName)
str := fmt.Sprintf(" Table '%s' can be interleaved with table '%s' %s '%s' and Column '%s'", spSchema.Name, parent, IssueDB[i].Brief, spSchema.Name, spColName)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -297,7 +274,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
case internal.InterleavedOrder:
parent, _, _ := getInterleaveDetail(conv, tableId, colId, i)
str := fmt.Sprintf("Table %s %s %s go to Interleave Table Tab", spSchema.Name, IssueDB[i].Brief, parent)
str := fmt.Sprintf("Table '%s' %s '%s' go to Interleave Table Tab", spSchema.Name, IssueDB[i].Brief, parent)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -308,7 +285,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
case internal.InterleavedAddColumn:
parent, _, _ := getInterleaveDetail(conv, tableId, colId, i)
str := fmt.Sprintf("Table '%s' is %s %s add %s as a primary key in table %s", conv.SpSchema[tableId].Name, IssueDB[i].Brief, parent, spColName, spSchema.Name)
str := fmt.Sprintf("Table '%s' is %s '%s' add '%s' as a primary key in table '%s'", conv.SpSchema[tableId].Name, IssueDB[i].Brief, parent, spColName, spSchema.Name)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -319,7 +296,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
case internal.InterleavedRenameColumn:
parent, fkName, referColName := getInterleaveDetail(conv, tableId, colId, i)
str := fmt.Sprintf(" %s %s rename %s primary key in table %s to match the foreign key %s refer column \"%s\"", IssueDB[i].Brief, parent, spColName, spSchema.Name, fkName, referColName)
str := fmt.Sprintf(" %s '%s' rename '%s' primary key in table '%s' to match the foreign key '%s' refer column '%s'", IssueDB[i].Brief, parent, spColName, spSchema.Name, fkName, referColName)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -330,7 +307,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
case internal.InterleavedChangeColumnSize:
parent, fkName, referColName := getInterleaveDetail(conv, tableId, colId, i)
str := fmt.Sprintf(" %s %s change column size of column %s primary key in table %s to match the foreign key %s refer column \"%s\"", IssueDB[i].Brief, parent, spColName, spSchema.Name, fkName, referColName)
str := fmt.Sprintf(" %s '%s' change column size of column '%s' primary key in table '%s' to match the foreign key '%s' refer column '%s'", IssueDB[i].Brief, parent, spColName, spSchema.Name, fkName, referColName)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -340,7 +317,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
l = append(l, toAppend)
}
case internal.RedundantIndex:
str := fmt.Sprintf(" %s for Table %s and Column %s", IssueDB[i].Brief, spSchema.Name, spColName)
str := fmt.Sprintf(" %s for Table '%s' and Column '%s'", IssueDB[i].Brief, spSchema.Name, spColName)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -351,7 +328,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}

case internal.AutoIncrementIndex:
str := fmt.Sprintf(" %s for Table %s and Column %s", IssueDB[i].Brief, spSchema.Name, spColName)
str := fmt.Sprintf(" %s for Table '%s' and Column '%s'", IssueDB[i].Brief, spSchema.Name, spColName)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -362,7 +339,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}

case internal.InterleaveIndex:
str := fmt.Sprintf("Column %s of Table %s %s", spColName, spSchema.Name, IssueDB[i].Brief)
str := fmt.Sprintf("Column '%s' of Table '%s' %s", spColName, spSchema.Name, IssueDB[i].Brief)

if !Contains(l, str) {
toAppend := Issue{
Expand All @@ -372,15 +349,15 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
l = append(l, toAppend)
}
case internal.ShardIdColumnAdded:
str := fmt.Sprintf("Table '%s': %s %s", conv.SpSchema[tableId].Name, conv.SpSchema[tableId].ColDefs[conv.SpSchema[tableId].ShardIdColumn].Name, IssueDB[i].Brief)
str := fmt.Sprintf("Table '%s': '%s' %s", conv.SpSchema[tableId].Name, conv.SpSchema[tableId].ColDefs[conv.SpSchema[tableId].ShardIdColumn].Name, IssueDB[i].Brief)
toAppend := Issue{
Category: IssueDB[i].Category,
Description: str,
}
l = append(l, toAppend)

case internal.ShardIdColumnPrimaryKey:
str := fmt.Sprintf("Table '%s': %s %s", conv.SpSchema[tableId].Name, conv.SpSchema[tableId].ColDefs[conv.SpSchema[tableId].ShardIdColumn].Name, IssueDB[i].Brief)
str := fmt.Sprintf("Table '%s': '%s' %s", conv.SpSchema[tableId].Name, conv.SpSchema[tableId].ColDefs[conv.SpSchema[tableId].ShardIdColumn].Name, IssueDB[i].Brief)
toAppend := Issue{
Category: IssueDB[i].Category,
Description: str,
Expand All @@ -396,11 +373,21 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
case internal.ArrayTypeNotSupported:
toAppend := Issue{
Category: IssueDB[i].Category,
Description: fmt.Sprintf("Table '%s': Column %s, %s", conv.SpSchema[tableId].Name, spColName, IssueDB[i].Brief),
Description: fmt.Sprintf("Table '%s': Column '%s', %s", conv.SpSchema[tableId].Name, spColName, IssueDB[i].Brief),
}
l = append(l, toAppend)
case internal.MissingPrimaryKey:
// We don't want default case to handle the missing primary key warning
toAppend := Issue{
Category: IssueDB[i].Category,
Description: fmt.Sprintf("Column '%s' was added because table '%s' didn't have a primary key. Spanner requires a primary key for every table", *syntheticPK, conv.SpSchema[tableId].Name),
}
l = append(l, toAppend)
case internal.UniqueIndexPrimaryKey:
toAppend := Issue{
Category: IssueDB[i].Category,
Description: fmt.Sprintf("UNIQUE constraint on column(s) '%s' replaced with primary key since table '%s' didn't have one. Spanner requires a primary key for every table", strings.Join(uniquePK, ", "), conv.SpSchema[tableId].Name),
}
l = append(l, toAppend)
default:
toAppend := Issue{
Category: IssueDB[i].Category,
Expand Down Expand Up @@ -456,8 +443,7 @@ func getInterleaveDetail(conv *internal.Conv, tableId string, colId string, issu
if err1 == nil && colPkOrder != 1 {
return conv.SpSchema[fk.ReferTableId].Name, "", ""
}
case internal.InterleavedRenameColumn:
case internal.InterleavedChangeColumnSize:
case internal.InterleavedRenameColumn, internal.InterleavedChangeColumnSize:
if err1 == nil {
parentTable := conv.SpSchema[fk.ReferTableId]
return conv.SpSchema[fk.ReferTableId].Name, fk.Name, parentTable.ColDefs[fk.ReferColumnIds[i]].Name
Expand Down Expand Up @@ -552,7 +538,9 @@ var IssueDB = map[internal.SchemaIssue]struct {
internal.ShardIdColumnPrimaryKey: {Brief: "column is not a part of primary key. You may go to the Primary Key tab and add this column as a part of Primary Key", severity: suggestion, Category: "SHARD_ID_ADD_COLUMN_PRIMARY_KEY",
CategoryDescription: "Shard id column is not a part of primary key. Please add it to primary key"},
internal.MissingPrimaryKey: {Category: "MISSING_PRIMARY_KEY",
CategoryDescription: "Primary Key is missing"},
CategoryDescription: "Primary Key is missing, synthetic column created as a primary key"},
internal.UniqueIndexPrimaryKey: {Category: "UNIQUE_INDEX_PRIMARY_KEY",
CategoryDescription: "Primary Key is missing, unique column(s) used as primary key"},
internal.ArrayTypeNotSupported: {Brief: "Array datatype migration is not fully supported. Please validate data after data migration", severity: warning, Category: "ARRAY_TYPE_NOT_SUPPORTED"},
}

Expand Down
16 changes: 8 additions & 8 deletions test_data/mysql_structured_report.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@
"issueType": "Warnings",
"issueList": [
{
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'bad_schema' didn't have a primary key. Spanner requires a primary key for every table"
"category": "STORAGE_WARNING",
"description": "Table 'bad_schema': Some columns will consume more storage in Spanner e.g. for column 'a', source DB type float is mapped to Spanner data type float64"
},
{
"category": "STORAGE_WARNING",
"description": "Table bad_schema: Some columns will consume more storage in Spanner e.g. for column 'a', source DB type float is mapped to Spanner data type float64"
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'bad_schema' didn't have a primary key. Spanner requires a primary key for every table"
}
]
}
Expand Down Expand Up @@ -149,12 +149,12 @@
"issueType": "Warnings",
"issueList": [
{
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'no_pk' didn't have a primary key. Spanner requires a primary key for every table"
"category": "STORAGE_WARNING",
"description": "Table 'no_pk': Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int(11) is mapped to Spanner data type int64"
},
{
"category": "STORAGE_WARNING",
"description": "Table no_pk: Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int(11) is mapped to Spanner data type int64"
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'no_pk' didn't have a primary key. Spanner requires a primary key for every table"
}
]
}
Expand Down
12 changes: 6 additions & 6 deletions test_data/mysql_text_report.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ Schema conversion: POOR ( 0% of 2 columns mapped cleanly) + missing primary key.
Data conversion: OK (94% of 1000 rows written to Spanner).

Warnings
1) Column 'synth_id' was added because table 'bad_schema' didn't have a primary
key. Spanner requires a primary key for every table.
2) Table bad_schema: Some columns will consume more storage in Spanner e.g. for
1) Table 'bad_schema': Some columns will consume more storage in Spanner e.g. for
column 'a', source DB type float is mapped to Spanner data type float64.
2) Column 'synth_id' was added because table 'bad_schema' didn't have a primary
key. Spanner requires a primary key for every table.

----------------------------
Table default_value
Expand Down Expand Up @@ -68,10 +68,10 @@ Schema conversion: POOR (33% of 3 columns mapped cleanly) + missing primary key.
Data conversion: POOR (60% of 5000 rows written to Spanner).

Warnings
1) Column 'synth_id' was added because table 'no_pk' didn't have a primary key.
1) Table 'no_pk': Some columns will consume more storage in Spanner e.g. for
column 'b', source DB type int(11) is mapped to Spanner data type int64.
2) Column 'synth_id' was added because table 'no_pk' didn't have a primary key.
Spanner requires a primary key for every table.
2) Table no_pk: Some columns will consume more storage in Spanner e.g. for column
'b', source DB type int(11) is mapped to Spanner data type int64.

----------------------------
Unexpected Conditions
Expand Down
22 changes: 11 additions & 11 deletions test_data/postgres_structured_report.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,9 @@
{
"issueType": "Warnings",
"issueList": [
{
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'bad_schema' didn't have a primary key. Spanner requires a primary key for every table"
},
{
"category": "STORAGE_WARNING",
"description": "Table bad_schema: Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int4 is mapped to Spanner data type int64"
"description": "Table 'bad_schema': Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int4 is mapped to Spanner data type int64"
},
{
"category": "MULTI_DIMENSIONAL_ARRAY_USES",
Expand All @@ -67,6 +63,10 @@
{
"category": "INAPPROPRIATE_TYPE",
"description": "Table 'bad_schema': Column 'd', type circle is mapped to string(max). No appropriate Spanner type. The column will be made nullable in Spanner"
},
{
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'bad_schema' didn't have a primary key. Spanner requires a primary key for every table"
}
]
}
Expand Down Expand Up @@ -156,17 +156,17 @@
{
"issueType": "Warnings",
"issueList": [
{
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'no_pk' didn't have a primary key. Spanner requires a primary key for every table"
},
{
"category": "ARRAY_TYPE_NOT_SUPPORTED",
"description": "Table 'no_pk': Column a, Array datatype migration is not fully supported. Please validate data after data migration"
"description": "Table 'no_pk': Column 'a', Array datatype migration is not fully supported. Please validate data after data migration"
},
{
"category": "STORAGE_WARNING",
"description": "Table no_pk: Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int4 is mapped to Spanner data type int64"
"description": "Table 'no_pk': Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int4 is mapped to Spanner data type int64"
},
{
"category": "MISSING_PRIMARY_KEY",
"description": "Column 'synth_id' was added because table 'no_pk' didn't have a primary key. Spanner requires a primary key for every table"
}
]
}
Expand Down
Loading

0 comments on commit dd43f5b

Please sign in to comment.