Skip to content

Commit

Permalink
Bug fixes and minor improvements (#580)
Browse files Browse the repository at this point in the history 
* minor changes

* some bug hunt fixes

* fixed bug hunt bugs

* ng build

* fixed failing tests

* addressed pr comments

* fixed test

* ng build files
  • Loading branch information
@shreyakhajanchi
shreyakhajanchi committed Jul 18, 2023
1 parent 455e727 commit ccd3089
Show file tree
Hide file tree
Showing 28 changed files with 343 additions and 202 deletions.
12 changes: 7 additions & 5 deletions internal/convert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ const (
ShardIdColumnPrimaryKey
)

const ShardIdColumn = "migration_shard_id"
const (
ShardIdColumn = "migration_shard_id"
SyntheticPrimaryKey = "synth_id"
)

// NameAndCols contains the name of a table and its columns.
// Used to map between source DB and Spanner table and column names.
Expand Down Expand Up @@ -362,7 +365,7 @@ func (conv *Conv) SampleBadRows(n int) []string {
func (conv *Conv) AddShardIdColumn() {
for t, ct := range conv.SpSchema {
if ct.ShardIdColumn == "" {
colName := ShardIdColumn
colName := conv.buildColumnNameWithBase(t, ShardIdColumn)
columnId := GenerateColumnId()
ct.ColIds = append(ct.ColIds, columnId)
ct.ColDefs[columnId] = ddl.ColumnDef{Name: colName, Id: columnId, T: ddl.Type{Name: ddl.String, Len: 50}, NotNull: false}
Expand Down Expand Up @@ -397,7 +400,7 @@ func (conv *Conv) AddPrimaryKeys() {
}
}
if !primaryKeyPopulated {
k := conv.buildPrimaryKey(t)
k := conv.buildColumnNameWithBase(t, SyntheticPrimaryKey)
columnId := GenerateColumnId()
ct.ColIds = append(ct.ColIds, columnId)
ct.ColDefs[columnId] = ddl.ColumnDef{Name: k, Id: columnId, T: ddl.Type{Name: ddl.String, Len: 50}}
Expand All @@ -414,8 +417,7 @@ func (conv *Conv) SetLocation(loc *time.Location) {
conv.Location = loc
}

func (conv *Conv) buildPrimaryKey(tableId string) string {
base := "synth_id"
func (conv *Conv) buildColumnNameWithBase(tableId, base string) string {
if _, ok := conv.SpSchema[tableId]; !ok {
conv.Unexpected(fmt.Sprintf("Table doesn't exist for tableId %s: ", tableId))
return base
Expand Down
36 changes: 18 additions & 18 deletions internal/reports/report_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,14 +130,14 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
// 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 {
l = append(l, fmt.Sprintf("Column '%s' was added because this table didn't have a primary key. Spanner requires a primary key for every table", *syntheticPK))
l = append(l, 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))
}
}
if uniquePK != nil {
// Warning about using a column with unique constraint as primary key
// in case primary key is absent.
if p.severity == warning {
l = append(l, fmt.Sprintf("UNIQUE constraint on column(s) '%s' replaced with primary key since this table didn't have one. Spanner requires a primary key for every table", strings.Join(uniquePK, ", ")))
l = append(l, 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))
}
}

Expand All @@ -149,7 +149,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
_, isChanged := internal.FixName(srcFk.Name)
if isChanged && srcFk.Name != spFk.Name {
l = append(l, fmt.Sprintf("%s, Foreign Key '%s' is mapped to '%s'", IssueDB[internal.IllegalName].Brief, srcFk.Name, spFk.Name))
l = append(l, fmt.Sprintf("%s, Foreign Key '%s' is mapped to '%s' for table '%s'", IssueDB[internal.IllegalName].Brief, srcFk.Name, spFk.Name, conv.SpSchema[tableId].Name))
}
}
for _, spIdx := range conv.SpSchema[tableId].Indexes {
Expand All @@ -159,7 +159,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
_, isChanged := internal.FixName(srcIdx.Name)
if isChanged && srcIdx.Name != spIdx.Name {
l = append(l, fmt.Sprintf("%s, Index '%s' is mapped to '%s'", IssueDB[internal.IllegalName].Brief, srcIdx.Name, spIdx.Name))
l = append(l, fmt.Sprintf("%s, Index '%s' is mapped to '%s' for table '%s'", IssueDB[internal.IllegalName].Brief, srcIdx.Name, spIdx.Name, conv.SpSchema[tableId].Name))
}
}

Expand Down Expand Up @@ -203,18 +203,18 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
spColType = strings.ToLower(spColType)
switch i {
case internal.DefaultValue:
l = append(l, fmt.Sprintf("%s e.g. column '%s'", IssueDB[i].Brief, spColName))
l = append(l, fmt.Sprintf("%s for table '%s' e.g. column '%s'", IssueDB[i].Brief, conv.SpSchema[tableId].Name, spColName))
case internal.ForeignKey:
l = append(l, fmt.Sprintf("Column '%s' uses foreign keys which HarbourBridge does not support yet", spColName))
l = append(l, fmt.Sprintf("Column '%s' in table '%s' uses foreign keys which HarbourBridge does not support yet", conv.SpSchema[tableId].Name, spColName))
case internal.AutoIncrement:
l = append(l, fmt.Sprintf("Column '%s' is an autoincrement column. %s", spColName, IssueDB[i].Brief))
l = append(l, fmt.Sprintf("Column '%s' is an autoincrement column in table '%s'. %s", spColName, conv.SpSchema[tableId].Name, IssueDB[i].Brief))
case internal.Timestamp:
// Avoid the confusing "timestamp is mapped to timestamp" message.
l = append(l, fmt.Sprintf("Some columns have source DB type 'timestamp without timezone' which is mapped to Spanner type timestamp e.g. column '%s'. %s", spColName, IssueDB[i].Brief))
l = append(l, fmt.Sprintf("Some columns have source DB type 'timestamp without timezone' which is mapped to Spanner type timestamp in table '%s' e.g. column '%s'. %s", conv.SpSchema[tableId].Name, spColName, IssueDB[i].Brief))
case internal.Datetime:
l = append(l, fmt.Sprintf("Some columns have source DB type 'datetime' which is mapped to Spanner type timestamp e.g. column '%s'. %s", spColName, IssueDB[i].Brief))
l = append(l, fmt.Sprintf("Some columns have source DB type 'datetime' which is mapped to Spanner type timestamp in table '%s' e.g. column '%s'. %s", conv.SpSchema[tableId].Name, spColName, IssueDB[i].Brief))
case internal.Widened:
l = append(l, fmt.Sprintf("%s e.g. for column '%s', source DB type %s is mapped to Spanner data type %s", IssueDB[i].Brief, spColName, srcColType, spColType))
l = append(l, 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))
case internal.HotspotTimestamp:
str := fmt.Sprintf(" %s for Table %s and Column %s", IssueDB[i].Brief, spSchema.Name, spColName)

Expand Down Expand Up @@ -243,7 +243,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}
case internal.InterleavedAddColumn:
parent, _, _ := getInterleaveDetail(conv, tableId, colId, i)
str := fmt.Sprintf(" %s %s add %s as a primary key in table %s", 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 !internal.Contains(l, str) {
l = append(l, str)
Expand Down Expand Up @@ -283,13 +283,13 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
l = append(l, str)
}
case internal.ShardIdColumnAdded:
l = append(l, IssueDB[i].Brief)
l = append(l, fmt.Sprintf("Table '%s': %s %s", conv.SpSchema[tableId].Name, conv.SpSchema[tableId].ColDefs[conv.SpSchema[tableId].ShardIdColumn].Name, IssueDB[i].Brief))
case internal.ShardIdColumnPrimaryKey:
l = append(l, IssueDB[i].Brief)
l = append(l, fmt.Sprintf("Table '%s': %s %s", conv.SpSchema[tableId].Name, conv.SpSchema[tableId].ColDefs[conv.SpSchema[tableId].ShardIdColumn].Name, IssueDB[i].Brief))
case internal.IllegalName:
l = append(l, fmt.Sprintf("%s, Column '%s' is mapped to '%s'", IssueDB[i].Brief, srcColName, spColName))
l = append(l, fmt.Sprintf("%s, Column '%s' is mapped to '%s' for table '%s'", IssueDB[i].Brief, srcColName, spColName, conv.SpSchema[tableId].Name))
default:
l = append(l, fmt.Sprintf("Column '%s': type %s is mapped to %s. %s", spColName, srcColType, spColType, IssueDB[i].Brief))
l = append(l, fmt.Sprintf("Table '%s': Column '%s', type %s is mapped to %s. %s", conv.SpSchema[tableId].Name, spColName, srcColType, spColType, IssueDB[i].Brief))
}
}
}
Expand Down Expand Up @@ -384,7 +384,7 @@ var IssueDB = map[internal.SchemaIssue]struct {
severity severity
batch bool // Whether multiple instances of this issue are combined.
}{
internal.DefaultValue: {Brief: "Some columns have default values which Spanner does not support", severity: warning, batch: true},
internal.DefaultValue: {Brief: "Some columns have default values which HarbourBridge does not migrate. Please add the default constraints manually after the migration is complete", severity: note, batch: true},
internal.ForeignKey: {Brief: "Spanner does not support foreign keys", severity: warning},
internal.MultiDimensionalArray: {Brief: "Spanner doesn't support multi-dimensional arrays", severity: warning},
internal.NoGoodType: {Brief: "No appropriate Spanner type", severity: warning},
Expand All @@ -411,8 +411,8 @@ var IssueDB = map[internal.SchemaIssue]struct {
internal.InterleavedRenameColumn: {Brief: "Candidate for Interleaved Table", severity: suggestion},
internal.InterleavedChangeColumnSize: {Brief: "Candidate for Interleaved Table", severity: suggestion},
internal.RowLimitExceeded: {Brief: "Non key columns exceed the spanner limit of 1600 MB. Please modify the column sizes", severity: errors},
internal.ShardIdColumnAdded: {Brief: "Column migration_shard_id was added because this is a sharded migration and this column cannot be dropped", severity: note},
internal.ShardIdColumnPrimaryKey: {Brief: "Column migration_shard_id 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},
internal.ShardIdColumnAdded: {Brief: "column was added because this is a sharded migration and this column cannot be dropped", severity: note},
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},
}

type severity int
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 @@ -50,8 +50,8 @@
{
"warningType":"Warnings",
"warningList":[
"Column 'synth_id' was added because this table didn't have a primary key. Spanner requires a primary key for every table",
"Some columns will consume more storage in Spanner e.g. for column 'a', source DB type float is mapped to Spanner data type float64"
"Column 'synth_id' was added because table 'bad_schema' didn't have a primary key. Spanner requires a primary key for every table",
"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"
]
}
]
Expand All @@ -60,9 +60,9 @@
"srcTableName":"default_value",
"spTableName":"default_value",
"schemaReport":{
"rating":"POOR",
"rating":"EXCELLENT",
"pkMissing":false,
"warnings":1,
"warnings":0,
"totalColumns":2
},
"dataReport":{
Expand All @@ -73,9 +73,9 @@
},
"warnings":[
{
"warningType":"Warning",
"warningType":"Note",
"warningList":[
"Some columns have default values which Spanner does not support e.g. column 'b'"
"Some columns have default values which HarbourBridge does not migrate. Please add the default constraints manually after the migration is complete for table 'default_value' e.g. column 'b'"
]
}
]
Expand Down Expand Up @@ -133,8 +133,8 @@
{
"warningType":"Warnings",
"warningList":[
"Column 'synth_id' was added because this table didn't have a primary key. Spanner requires a primary key for every table",
"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"
"Column 'synth_id' was added because table 'no_pk' didn't have a primary key. Spanner requires a primary key for every table",
"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"
]
}
]
Expand Down
23 changes: 12 additions & 11 deletions test_data/mysql_text_report.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,21 @@ Schema conversion: POOR (50% 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 this table didn't have a primary key.
Spanner requires a primary key for every table.
2) Some columns will consume more storage in Spanner e.g. for column 'a', source
DB type float is mapped to Spanner data type float64.
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
column 'a', source DB type float is mapped to Spanner data type float64.

----------------------------
Table default_value
----------------------------
Schema conversion: POOR (50% of 2 columns mapped cleanly).
Schema conversion: EXCELLENT (100% of 2 columns mapped cleanly).
Data conversion: NONE (100% of 0 rows written to Spanner).

Warning
1) Some columns have default values which Spanner does not support e.g. column
'b'.
Note
1) Some columns have default values which HarbourBridge does not migrate. Please
add the default constraints manually after the migration is complete for table
'default_value' e.g. column 'b'.

----------------------------
Table excellent_schema
Expand All @@ -67,10 +68,10 @@ Schema conversion: POOR (67% 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 this table didn't have a primary key.
1) Column 'synth_id' was added because table 'no_pk' didn't have a primary key.
Spanner requires a primary key for every table.
2) 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) 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
20 changes: 10 additions & 10 deletions test_data/postgres_structured_report.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@
{
"warningType":"Warnings",
"warningList":[
"Column 'synth_id' was added because this table didn't have a primary key. Spanner requires a primary key for every table",
"Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int4 is mapped to Spanner data type int64",
"Column 'c': type int4[4][2] is mapped to string(max). Spanner doesn't support multi-dimensional arrays",
"Column 'd': type circle is mapped to string(max). No appropriate Spanner type"
"Column 'synth_id' was added because table 'bad_schema' didn't have a primary key. Spanner requires a primary key for every table",
"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",
"Table 'bad_schema': Column 'c', type int4[4][2] is mapped to string(max). Spanner doesn't support multi-dimensional arrays",
"Table 'bad_schema': Column 'd', type circle is mapped to string(max). No appropriate Spanner type"
]
}
]
Expand All @@ -62,9 +62,9 @@
"srcTableName":"default_value",
"spTableName":"default_value",
"schemaReport":{
"rating":"POOR",
"rating":"EXCELLENT",
"pkMissing":false,
"warnings":1,
"warnings":0,
"totalColumns":2
},
"dataReport":{
Expand All @@ -75,9 +75,9 @@
},
"warnings":[
{
"warningType":"Warning",
"warningType":"Note",
"warningList":[
"Some columns have default values which Spanner does not support e.g. column 'b'"
"Some columns have default values which HarbourBridge does not migrate. Please add the default constraints manually after the migration is complete for table 'default_value' e.g. column 'b'"
]
}
]
Expand Down Expand Up @@ -135,8 +135,8 @@
{
"warningType":"Warnings",
"warningList":[
"Column 'synth_id' was added because this table didn't have a primary key. Spanner requires a primary key for every table",
"Some columns will consume more storage in Spanner e.g. for column 'b', source DB type int4 is mapped to Spanner data type int64"
"Column 'synth_id' was added because table 'no_pk' didn't have a primary key. Spanner requires a primary key for every table",
"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"
]
}
]
Expand Down
31 changes: 16 additions & 15 deletions test_data/postgres_text_report.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,25 @@ Schema conversion: POOR (25% of 4 columns mapped cleanly) + missing primary key.
Data conversion: OK (94% of 1000 rows written to Spanner).

Warnings
1) Column 'synth_id' was added because this table didn't have a primary key.
Spanner requires a primary key for every table.
2) Some columns will consume more storage in Spanner e.g. for column 'b', source
DB type int4 is mapped to Spanner data type int64.
3) Column 'c': type int4[4][2] is mapped to string(max). Spanner doesn't support
multi-dimensional arrays.
4) Column 'd': type circle is mapped to string(max). No appropriate Spanner
type.
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
column 'b', source DB type int4 is mapped to Spanner data type int64.
3) Table 'bad_schema': Column 'c', type int4[4][2] is mapped to string(max).
Spanner doesn't support multi-dimensional arrays.
4) Table 'bad_schema': Column 'd', type circle is mapped to string(max). No
appropriate Spanner type.

----------------------------
Table default_value
----------------------------
Schema conversion: POOR (50% of 2 columns mapped cleanly).
Schema conversion: EXCELLENT (100% of 2 columns mapped cleanly).
Data conversion: NONE (100% of 0 rows written to Spanner).

Warning
1) Some columns have default values which Spanner does not support e.g. column
'b'.
Note
1) Some columns have default values which HarbourBridge does not migrate. Please
add the default constraints manually after the migration is complete for table
'default_value' e.g. column 'b'.

----------------------------
Table excellent_schema
Expand All @@ -70,10 +71,10 @@ Schema conversion: POOR (67% 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 this table didn't have a primary key.
1) Column 'synth_id' was added because table 'no_pk' didn't have a primary key.
Spanner requires a primary key for every table.
2) Some columns will consume more storage in Spanner e.g. for column 'b', source
DB type int4 is mapped to Spanner data type int64.
2) 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.

----------------------------
Unexpected Conditions
Expand Down
Loading

0 comments on commit ccd3089

Please sign in to comment.