Repository: cloudspannerecosystem/harbourbridge (go)

Issue:
TargetDB handling in getDDL() calls
func (s Schema) GetDDL(c Config) takes a config that accepts targetDB in the struct which is left out in a few places like the WriteSchemaFile() causing the generated schema files to be of GOOGLESQL dialect when specified dialect is PG.

ALso, FK statements still use backticks `` instead of "" for PG spanner. Check if any other handling is required.

Pull request description:
Pg spanner misc fixes
Fixes #241, #244 and #245 

- Added handling for float and null values
- For pg dump, null values were not getting skipped earlier.
- Quoting was incorrect for pg dialect, causing foreign key statements to fail
- The schema files were of spanner dialect since targetdb was not passed as a param.

Patch:
diff --git a/conversion/conversion.go b/conversion/conversion.go
index 3ce00a1..f4c2975 100644
--- a/conversion/conversion.go
+++ b/conversion/conversion.go
@@ -578,7 +578,7 @@ func UpdateDatabase(ctx context.Context, adminClient *database.DatabaseAdminClie
 	// Spanner DDL doesn't accept them), and protects table and col names
 	// using backticks (to avoid any issues with Spanner reserved words).
 	// Foreign Keys are set to false since we create them post data migration.
-	schema := conv.SpSchema.GetDDL(ddl.Config{Comments: false, ProtectIds: false, Tables: true, ForeignKeys: false, TargetDb: conv.TargetDb})
+	schema := conv.SpSchema.GetDDL(ddl.Config{Comments: false, ProtectIds: true, Tables: true, ForeignKeys: false, TargetDb: conv.TargetDb})
 	req := &adminpb.UpdateDatabaseDdlRequest{
 		Database:   dbURI,
 		Statements: schema,
@@ -633,7 +633,7 @@ func UpdateDDLForeignKeys(ctx context.Context, adminClient *database.DatabaseAdm
 	// The schema we send to Spanner excludes comments (since Cloud
 	// Spanner DDL doesn't accept them), and protects table and col names
 	// using backticks (to avoid any issues with Spanner reserved words).
-	fkStmts := conv.SpSchema.GetDDL(ddl.Config{Comments: false, ProtectIds: true, Tables: false, ForeignKeys: true})
+	fkStmts := conv.SpSchema.GetDDL(ddl.Config{Comments: false, ProtectIds: true, Tables: false, ForeignKeys: true, TargetDb: conv.TargetDb})
 	if len(fkStmts) == 0 {
 		return nil
 	}
@@ -778,7 +778,7 @@ func WriteSchemaFile(conv *internal.Conv, now time.Time, name string, out *os.Fi
 	// and doesn't add backticks around table and column names. This file is
 	// intended for explanatory and documentation purposes, and is not strictly
 	// legal Cloud Spanner DDL (Cloud Spanner doesn't currently support comments).
-	spDDL := conv.SpSchema.GetDDL(ddl.Config{Comments: true, ProtectIds: false, Tables: true, ForeignKeys: true})
+	spDDL := conv.SpSchema.GetDDL(ddl.Config{Comments: true, ProtectIds: false, Tables: true, ForeignKeys: true, TargetDb: conv.TargetDb})
 	if len(spDDL) == 0 {
 		spDDL = []string{"\n-- Schema is empty -- no tables found\n"}
 	}
@@ -805,7 +805,7 @@ func WriteSchemaFile(conv *internal.Conv, now time.Time, name string, out *os.Fi
 
 	// We change 'Comments' to false and 'ProtectIds' to true below to write out a
 	// schema file that is a legal Cloud Spanner DDL.
-	spDDL = conv.SpSchema.GetDDL(ddl.Config{Comments: false, ProtectIds: true, Tables: true, ForeignKeys: true})
+	spDDL = conv.SpSchema.GetDDL(ddl.Config{Comments: false, ProtectIds: true, Tables: true, ForeignKeys: true, TargetDb: conv.TargetDb})
 	if len(spDDL) == 0 {
 		spDDL = []string{"\n-- Schema is empty -- no tables found\n"}
 	}
diff --git a/sources/postgres/data.go b/sources/postgres/data.go
index da3d237..a839281 100644
--- a/sources/postgres/data.go
+++ b/sources/postgres/data.go
@@ -76,7 +76,10 @@ func ConvertData(conv *internal.Conv, srcTable string, srcCols []string, vals []
 	}
 	for i, spCol := range spCols {
 		srcCol := srcCols[i]
-		if vals[i] == "\\N" { // PostgreSQL representation of empty column in COPY-FROM blocks.
+		// "\\N" is for PostgreSQL representation of empty column in COPY-FROM blocks.
+		// TODO: Consider using NullString to differentiate between an actual column having "NULL" as a string
+		// and NULL values.
+		if vals[i] == "\\N" || vals[i] == "NULL" {
 			continue
 		}
 		spColDef, ok1 := spSchema.ColDefs[spCol]
diff --git a/sources/postgres/pgdump.go b/sources/postgres/pgdump.go
index 4318ae8..f64cc1f 100644
--- a/sources/postgres/pgdump.go
+++ b/sources/postgres/pgdump.go
@@ -724,6 +724,9 @@ func getRows(conv *internal.Conv, vll []*pg_query.Node, n *pg_query.InsertStmt)
 				switch val := v.GetNode().(type) {
 				case *pg_query.Node_AConst:
 					switch c := val.AConst.Val.GetNode().(type) {
+					// Most data is dumped enclosed in quotes ('') lke 'abc', '12:30:45' etc which is classified
+					// as type Node_String_ by the parser. Some data might not be quoted like (NULL, 14.67) and
+					// the type assigned to them is Node_Null and Node_Float respectively.
 					case *pg_query.Node_String_:
 						values = append(values, trimString(c.String_))
 					case *pg_query.Node_Integer:
@@ -732,6 +735,12 @@ func getRows(conv *internal.Conv, vll []*pg_query.Node, n *pg_query.InsertStmt)
 						// high priority (it isn't right now), then consider preserving int64
 						// here to avoid the int64 -> string -> int64 conversions.
 						values = append(values, strconv.FormatInt(int64(c.Integer.Ival), 10))
+					case *pg_query.Node_Float:
+						values = append(values, c.Float.Str)
+					case *pg_query.Node_Null:
+						values = append(values, "NULL")
+					// TODO: There might be other Node types like Node_IntList, Node_List, Node_BitString etc that
+					// need to be checked if they are handled or not.
 					default:
 						conv.Unexpected(fmt.Sprintf("Processing %v statement: found %s node for A_Const Val", printNodeType(n), printNodeType(c)))
 					}
diff --git a/spanner/ddl/ast.go b/spanner/ddl/ast.go
index ea6a72f..3f5cc05 100644
--- a/spanner/ddl/ast.go
+++ b/spanner/ddl/ast.go
@@ -158,7 +158,11 @@ type Config struct {
 
 func (c Config) quote(s string) string {
 	if c.ProtectIds {
-		return "`" + s + "`"
+		if c.TargetDb == constants.TargetExperimentalPostgres {
+			return "\"" + s + "\""
+		} else {
+			return "`" + s + "`"
+		}
 	}
 	return s
 }


Test patch:
diff --git a/spanner/ddl/ast_test.go b/spanner/ddl/ast_test.go
index cfaafce..2476702 100644
--- a/spanner/ddl/ast_test.go
+++ b/spanner/ddl/ast_test.go
@@ -88,7 +88,7 @@ func TestPrintColumnDefPG(t *testing.T) {
 		{in: ColumnDef{Name: "col1", T: Type{Name: Int64, IsArray: true}}, expected: "col1 VARCHAR(2621440)"},
 		{in: ColumnDef{Name: "col1", T: Type{Name: Int64}, NotNull: true}, expected: "col1 INT8 NOT NULL"},
 		{in: ColumnDef{Name: "col1", T: Type{Name: Int64, IsArray: true}, NotNull: true}, expected: "col1 VARCHAR(2621440) NOT NULL"},
-		{in: ColumnDef{Name: "col1", T: Type{Name: Int64}}, protectIds: true, expected: "`col1` INT8"},
+		{in: ColumnDef{Name: "col1", T: Type{Name: Int64}}, protectIds: true, expected: "\"col1\" INT8"},
 	}
 	for _, tc := range tests {
 		s, _ := tc.in.PrintColumnDef(Config{ProtectIds: tc.protectIds, TargetDb: constants.TargetExperimentalPostgres})
@@ -100,15 +100,16 @@ func TestPrintIndexKey(t *testing.T) {
 	tests := []struct {
 		in         IndexKey
 		protectIds bool
+		targetDb   string
 		expected   string
 	}{
 		{in: IndexKey{Col: "col1"}, expected: "col1"},
 		{in: IndexKey{Col: "col1", Desc: true}, expected: "col1 DESC"},
 		{in: IndexKey{Col: "col1"}, protectIds: true, expected: "`col1`"},
+		{in: IndexKey{Col: "col1"}, protectIds: true, targetDb: constants.TargetExperimentalPostgres, expected: "\"col1\""},
 	}
 	for _, tc := range tests {
-		assert.Equal(t, tc.expected, tc.in.PrintIndexKey(Config{ProtectIds: tc.protectIds}))
-		assert.Equal(t, tc.expected, tc.in.PrintIndexKey(Config{ProtectIds: tc.protectIds, TargetDb: constants.TargetExperimentalPostgres}))
+		assert.Equal(t, tc.expected, tc.in.PrintIndexKey(Config{ProtectIds: tc.protectIds, TargetDb: tc.targetDb}))
 	}
 }
 
@@ -226,11 +227,11 @@ func TestPrintCreateTablePG(t *testing.T) {
 			"quote",
 			true,
 			t1,
-			"CREATE TABLE `mytable` (\n" +
-				"	`col1` INT8 NOT NULL,\n" +
-				"	`col2` VARCHAR(2621440),\n" +
-				"	`col3` BYTEA,\n" +
-				"	PRIMARY KEY (`col1` DESC)\n" +
+			"CREATE TABLE \"mytable\" (\n" +
+				"	\"col1\" INT8 NOT NULL,\n" +
+				"	\"col2\" VARCHAR(2621440),\n" +
+				"	\"col3\" BYTEA,\n" +
+				"	PRIMARY KEY (\"col1\" DESC)\n" +
 				")",
 		},
 		{
@@ -267,16 +268,18 @@ func TestPrintCreateIndex(t *testing.T) {
 	tests := []struct {
 		name       string
 		protectIds bool
+		targetDb   string
 		index      CreateIndex
 		expected   string
 	}{
-		{"no quote non unique", false, ci[0], "CREATE INDEX myindex ON mytable (col1 DESC, col2)"},
-		{"quote non unique", true, ci[0], "CREATE INDEX `myindex` ON `mytable` (`col1` DESC, `col2`)"},
-		{"unique key", true, ci[1], "CREATE UNIQUE INDEX `myindex2` ON `mytable` (`col1` DESC, `col2`)"},
+		{"no quote non unique", false, "", ci[0], "CREATE INDEX myindex ON mytable (col1 DESC, col2)"},
+		{"quote non unique", true, "", ci[0], "CREATE INDEX `myindex` ON `mytable` (`col1` DESC, `col2`)"},
+		{"unique key", true, "", ci[1], "CREATE UNIQUE INDEX `myindex2` ON `mytable` (`col1` DESC, `col2`)"},
+		{"quote non unique PG", true, constants.TargetExperimentalPostgres, ci[0], "CREATE INDEX \"myindex\" ON \"mytable\" (\"col1\" DESC, \"col2\")"},
+		{"unique key PG", true, constants.TargetExperimentalPostgres, ci[1], "CREATE UNIQUE INDEX \"myindex2\" ON \"mytable\" (\"col1\" DESC, \"col2\")"},
 	}
 	for _, tc := range tests {
-		assert.Equal(t, tc.expected, tc.index.PrintCreateIndex(Config{ProtectIds: tc.protectIds}))
-		assert.Equal(t, tc.expected, tc.index.PrintCreateIndex(Config{ProtectIds: tc.protectIds, TargetDb: constants.TargetExperimentalPostgres}))
+		assert.Equal(t, tc.expected, tc.index.PrintCreateIndex(Config{ProtectIds: tc.protectIds, TargetDb: tc.targetDb}))
 	}
 }
 
@@ -298,16 +301,17 @@ func TestPrintForeignKey(t *testing.T) {
 	tests := []struct {
 		name       string
 		protectIds bool
+		targetDb   string
 		expected   string
 		fk         Foreignkey
 	}{
-		{"no quote", false, "CONSTRAINT fk_test FOREIGN KEY (c1, c2) REFERENCES ref_table (ref_c1, ref_c2)", fk[0]},
-		{"quote", true, "CONSTRAINT `fk_test` FOREIGN KEY (`c1`, `c2`) REFERENCES `ref_table` (`ref_c1`, `ref_c2`)", fk[0]},
-		{"no constraint name", false, "FOREIGN KEY (c1) REFERENCES ref_table (ref_c1)", fk[1]},
+		{"no quote", false, "", "CONSTRAINT fk_test FOREIGN KEY (c1, c2) REFERENCES ref_table (ref_c1, ref_c2)", fk[0]},
+		{"quote", true, "", "CONSTRAINT `fk_test` FOREIGN KEY (`c1`, `c2`) REFERENCES `ref_table` (`ref_c1`, `ref_c2`)", fk[0]},
+		{"no constraint name", false, "", "FOREIGN KEY (c1) REFERENCES ref_table (ref_c1)", fk[1]},
+		{"quote PG", true, constants.TargetExperimentalPostgres, "CONSTRAINT \"fk_test\" FOREIGN KEY (\"c1\", \"c2\") REFERENCES \"ref_table\" (\"ref_c1\", \"ref_c2\")", fk[0]},
 	}
 	for _, tc := range tests {
-		assert.Equal(t, tc.expected, tc.fk.PrintForeignKey(Config{ProtectIds: tc.protectIds}))
-		assert.Equal(t, tc.expected, tc.fk.PrintForeignKey(Config{ProtectIds: tc.protectIds, TargetDb: constants.TargetExperimentalPostgres}))
+		assert.Equal(t, tc.expected, tc.fk.PrintForeignKey(Config{ProtectIds: tc.protectIds, TargetDb: tc.targetDb}))
 	}
 }
 
@@ -330,16 +334,17 @@ func TestPrintForeignKeyAlterTable(t *testing.T) {
 		name       string
 		table      string
 		protectIds bool
+		targetDb   string
 		expected   string
 		fk         Foreignkey
 	}{
-		{"no quote", "table1", false, "ALTER TABLE table1 ADD CONSTRAINT fk_test FOREIGN KEY (c1, c2) REFERENCES ref_table (ref_c1, ref_c2)", fk[0]},
-		{"quote", "table1", true, "ALTER TABLE `table1` ADD CONSTRAINT `fk_test` FOREIGN KEY (`c1`, `c2`) REFERENCES `ref_table` (`ref_c1`, `ref_c2`)", fk[0]},
-		{"no constraint name", "table1", false, "ALTER TABLE table1 ADD FOREIGN KEY (c1) REFERENCES ref_table (ref_c1)", fk[1]},
+		{"no quote", "table1", false, "", "ALTER TABLE table1 ADD CONSTRAINT fk_test FOREIGN KEY (c1, c2) REFERENCES ref_table (ref_c1, ref_c2)", fk[0]},
+		{"quote", "table1", true, "", "ALTER TABLE `table1` ADD CONSTRAINT `fk_test` FOREIGN KEY (`c1`, `c2`) REFERENCES `ref_table` (`ref_c1`, `ref_c2`)", fk[0]},
+		{"no constraint name", "table1", false, "", "ALTER TABLE table1 ADD FOREIGN KEY (c1) REFERENCES ref_table (ref_c1)", fk[1]},
+		{"quote PG", "table1", true, constants.TargetExperimentalPostgres, "ALTER TABLE \"table1\" ADD CONSTRAINT \"fk_test\" FOREIGN KEY (\"c1\", \"c2\") REFERENCES \"ref_table\" (\"ref_c1\", \"ref_c2\")", fk[0]},
 	}
 	for _, tc := range tests {
-		assert.Equal(t, tc.expected, tc.fk.PrintForeignKeyAlterTable(Config{ProtectIds: tc.protectIds}, tc.table))
-		assert.Equal(t, tc.expected, tc.fk.PrintForeignKeyAlterTable(Config{ProtectIds: tc.protectIds, TargetDb: constants.TargetExperimentalPostgres}, tc.table))
+		assert.Equal(t, tc.expected, tc.fk.PrintForeignKeyAlterTable(Config{ProtectIds: tc.protectIds, TargetDb: tc.targetDb}, tc.table))
 	}
 }