[sql] Kill NameArgs.Escape (#484)

artie-labs · Apr 22, 2024 · 1136a3d · 1136a3d
1 parent 32a107e
commit 1136a3d
Show file tree

Hide file tree

Showing 18 changed files with 20 additions and 66 deletions.
diff --git a/clients/bigquery/tableid.go b/clients/bigquery/tableid.go
@@ -45,6 +45,6 @@ func (ti TableIdentifier) FullyQualifiedName() string {
 		"`%s`.`%s`.%s",
 		ti.projectID,
 		ti.dataset,
-		sql.EscapeNameIfNecessary(ti.table, false, &sql.NameArgs{Escape: true, DestKind: constants.BigQuery}),
+		sql.EscapeNameIfNecessary(ti.table, false, &sql.NameArgs{DestKind: constants.BigQuery}),
 	)
 }
diff --git a/clients/mssql/tableid.go b/clients/mssql/tableid.go
@@ -33,6 +33,6 @@ func (ti TableIdentifier) FullyQualifiedName() string {
 	return fmt.Sprintf(
 		"%s.%s",
 		ti.schema,
-		sql.EscapeNameIfNecessary(ti.table, false, &sql.NameArgs{Escape: true, DestKind: constants.MSSQL}),
+		sql.EscapeNameIfNecessary(ti.table, false, &sql.NameArgs{DestKind: constants.MSSQL}),
 	)
 }
diff --git a/clients/redshift/tableid.go b/clients/redshift/tableid.go
@@ -36,6 +36,6 @@ func (ti TableIdentifier) FullyQualifiedName() string {
 	return fmt.Sprintf(
 		"%s.%s",
 		ti.schema,
-		sql.EscapeNameIfNecessary(ti.table, ti.uppercaseEscapedNames, &sql.NameArgs{Escape: true, DestKind: constants.Redshift}),
+		sql.EscapeNameIfNecessary(ti.table, ti.uppercaseEscapedNames, &sql.NameArgs{DestKind: constants.Redshift}),
 	)
 }
diff --git a/clients/shared/merge.go b/clients/shared/merge.go
@@ -123,7 +123,7 @@ func Merge(dwh destination.DataWarehouse, tableData *optimization.TableData, cfg
 		TableID:             tableID,
 		SubQuery:            subQuery,
 		IdempotentKey:       tableData.TopicConfig().IdempotentKey,
-		PrimaryKeys:         tableData.PrimaryKeys(dwh.ShouldUppercaseEscapedNames(), &sql.NameArgs{Escape: true, DestKind: dwh.Label()}),
+		PrimaryKeys:         tableData.PrimaryKeys(dwh.ShouldUppercaseEscapedNames(), &sql.NameArgs{DestKind: dwh.Label()}),
 		Columns:             tableData.ReadOnlyInMemoryCols(),
 		SoftDelete:          tableData.TopicConfig().SoftDelete,
 		DestKind:            dwh.Label(),

diff --git a/clients/shared/utils.go b/clients/shared/utils.go
@@ -32,7 +32,7 @@ func BackfillColumn(cfg config.Config, dwh destination.DataWarehouse, column col
 		return fmt.Errorf("failed to escape default value: %w", err)
 	}
 
-	escapedCol := column.Name(dwh.ShouldUppercaseEscapedNames(), &sql.NameArgs{Escape: true, DestKind: dwh.Label()})
+	escapedCol := column.Name(dwh.ShouldUppercaseEscapedNames(), &sql.NameArgs{DestKind: dwh.Label()})
 
 	// TODO: This is added because `default` is not technically a column that requires escaping, but it is required when it's in the where clause.
 	// Once we escape everything by default, we can remove this patch of code.

diff --git a/clients/snowflake/staging.go b/clients/snowflake/staging.go
@@ -87,7 +87,6 @@ func (s *Store) PrepareTemporaryTable(tableData *optimization.TableData, tableCo
 	copyCommand := fmt.Sprintf("COPY INTO %s (%s) FROM (SELECT %s FROM @%s)",
 		tempTableID.FullyQualifiedName(),
 		strings.Join(tableData.ReadOnlyInMemoryCols().GetColumnsToUpdate(s.ShouldUppercaseEscapedNames(), &sql.NameArgs{
-			Escape:   true,
 			DestKind: s.Label(),
 		}), ","),
 		escapeColumns(tableData.ReadOnlyInMemoryCols(), ","), addPrefixToTableName(tempTableID, "%"))

diff --git a/clients/snowflake/tableid.go b/clients/snowflake/tableid.go
@@ -45,6 +45,6 @@ func (ti TableIdentifier) FullyQualifiedName() string {
 		"%s.%s.%s",
 		ti.database,
 		ti.schema,
-		sql.EscapeNameIfNecessary(ti.table, ti.uppercaseEscapedNames, &sql.NameArgs{Escape: true, DestKind: constants.Snowflake}),
+		sql.EscapeNameIfNecessary(ti.table, ti.uppercaseEscapedNames, &sql.NameArgs{DestKind: constants.Snowflake}),
 	)
 }
diff --git a/lib/destination/ddl/ddl.go b/lib/destination/ddl/ddl.go
@@ -107,7 +107,6 @@ func (a AlterTableArgs) AlterTable(cols ...columns.Column) error {
 		switch a.ColumnOp {
 		case constants.Add:
 			colName := col.Name(*a.UppercaseEscNames, &sql.NameArgs{
-				Escape:   true,
 				DestKind: a.Dwh.Label(),
 			})
 
@@ -119,7 +118,6 @@ func (a AlterTableArgs) AlterTable(cols ...columns.Column) error {
 			colSQLParts = append(colSQLParts, fmt.Sprintf(`%s %s`, colName, typing.KindToDWHType(col.KindDetails, a.Dwh.Label(), col.PrimaryKey())))
 		case constants.Delete:
 			colSQLParts = append(colSQLParts, col.Name(*a.UppercaseEscNames, &sql.NameArgs{
-				Escape:   true,
 				DestKind: a.Dwh.Label(),
 			}))
 		}

diff --git a/lib/destination/ddl/ddl_bq_test.go b/lib/destination/ddl/ddl_bq_test.go
@@ -93,7 +93,6 @@ func (d *DDLTestSuite) TestAlterTableDropColumnsBigQuery() {
 		assert.NoError(d.T(), alterTableArgs.AlterTable(column))
 		query, _ := d.fakeBigQueryStore.ExecArgsForCall(callIdx)
 		assert.Equal(d.T(), fmt.Sprintf("ALTER TABLE %s drop COLUMN %s", fqName, column.Name(false, &artieSQL.NameArgs{
-			Escape:   true,
 			DestKind: d.bigQueryStore.Label(),
 		})), query)
 		callIdx += 1
@@ -154,7 +153,6 @@ func (d *DDLTestSuite) TestAlterTableAddColumns() {
 		assert.NoError(d.T(), alterTableArgs.AlterTable(col))
 		query, _ := d.fakeBigQueryStore.ExecArgsForCall(callIdx)
 		assert.Equal(d.T(), fmt.Sprintf("ALTER TABLE %s %s COLUMN %s %s", fqName, constants.Add, col.Name(false, &artieSQL.NameArgs{
-			Escape:   true,
 			DestKind: d.bigQueryStore.Label(),
 		}), typing.KindToDWHType(kind, d.bigQueryStore.Label(), false)), query)
 		callIdx += 1
@@ -216,7 +214,6 @@ func (d *DDLTestSuite) TestAlterTableAddColumnsSomeAlreadyExist() {
 		assert.NoError(d.T(), alterTableArgs.AlterTable(column))
 		query, _ := d.fakeBigQueryStore.ExecArgsForCall(callIdx)
 		assert.Equal(d.T(), fmt.Sprintf("ALTER TABLE %s %s COLUMN %s %s", fqName, constants.Add, column.Name(false, &artieSQL.NameArgs{
-			Escape:   true,
 			DestKind: d.bigQueryStore.Label(),
 		}), typing.KindToDWHType(column.KindDetails, d.bigQueryStore.Label(), false)), query)
 		callIdx += 1

diff --git a/lib/destination/ddl/ddl_sflk_test.go b/lib/destination/ddl/ddl_sflk_test.go
@@ -50,7 +50,6 @@ func (d *DDLTestSuite) TestAlterComplexObjects() {
 	for i := 0; i < len(cols); i++ {
 		execQuery, _ := d.fakeSnowflakeStagesStore.ExecArgsForCall(i)
 		assert.Equal(d.T(), fmt.Sprintf("ALTER TABLE %s add COLUMN %s %s", fqTable, cols[i].Name(false, &sql.NameArgs{
-			Escape:   true,
 			DestKind: d.snowflakeStagesStore.Label(),
 		}),
 			typing.KindToDWHType(cols[i].KindDetails, d.snowflakeStagesStore.Label(), false)), execQuery)
@@ -186,7 +185,7 @@ func (d *DDLTestSuite) TestAlterTableDeleteDryRun() {
 
 		execArg, _ := d.fakeSnowflakeStagesStore.ExecArgsForCall(i)
 		assert.Equal(d.T(), execArg, fmt.Sprintf("ALTER TABLE %s %s COLUMN %s", fqTable, constants.Delete,
-			cols[i].Name(false, &sql.NameArgs{Escape: true, DestKind: d.snowflakeStagesStore.Label()})))
+			cols[i].Name(false, &sql.NameArgs{DestKind: d.snowflakeStagesStore.Label()})))
 	}
 }
 

diff --git a/lib/destination/dml/merge.go b/lib/destination/dml/merge.go
@@ -99,7 +99,6 @@ func (m *MergeArgument) GetParts() ([]string, error) {
 	}
 
 	cols := m.Columns.GetColumnsToUpdate(*m.UppercaseEscNames, &sql.NameArgs{
-		Escape:   true,
 		DestKind: m.DestKind,
 	})
 
@@ -234,7 +233,6 @@ func (m *MergeArgument) GetStatement() (string, error) {
 	}
 
 	cols := m.Columns.GetColumnsToUpdate(*m.UppercaseEscNames, &sql.NameArgs{
-		Escape:   true,
 		DestKind: m.DestKind,
 	})
 
@@ -306,7 +304,6 @@ func (m *MergeArgument) GetMSSQLStatement() (string, error) {
 	}
 
 	cols := m.Columns.GetColumnsToUpdate(*m.UppercaseEscNames, &sql.NameArgs{
-		Escape:   true,
 		DestKind: m.DestKind,
 	})
 

diff --git a/lib/destination/dml/merge_parts_test.go b/lib/destination/dml/merge_parts_test.go
@@ -49,13 +49,11 @@ func getBasicColumnsForTest(compositeKey bool, uppercaseEscNames bool) result {
 
 	var pks []columns.Wrapper
 	pks = append(pks, columns.NewWrapper(idCol, uppercaseEscNames, &sql.NameArgs{
-		Escape:   true,
 		DestKind: constants.Redshift,
 	}))
 
 	if compositeKey {
 		pks = append(pks, columns.NewWrapper(emailCol, uppercaseEscNames, &sql.NameArgs{
-			Escape:   true,
 			DestKind: constants.Redshift,
 		}))
 	}

diff --git a/lib/destination/dml/merge_test.go b/lib/destination/dml/merge_test.go
@@ -250,11 +250,9 @@ func TestMergeStatementEscapePrimaryKeys(t *testing.T) {
 		IdempotentKey: "",
 		PrimaryKeys: []columns.Wrapper{
 			columns.NewWrapper(columns.NewColumn("id", typing.Invalid), false, &sql.NameArgs{
-				Escape:   true,
 				DestKind: constants.Snowflake,
 			}),
 			columns.NewWrapper(columns.NewColumn("group", typing.Invalid), false, &sql.NameArgs{
-				Escape:   true,
 				DestKind: constants.Snowflake,
 			}),
 		},

diff --git a/lib/sql/escape.go b/lib/sql/escape.go
@@ -10,15 +10,15 @@ import (
 )
 
 type NameArgs struct {
-	Escape   bool
 	DestKind constants.DestinationKind
 }
 
 // symbolsToEscape are additional keywords that we need to escape
 var symbolsToEscape = []string{":"}
 
 func EscapeNameIfNecessary(name string, uppercaseEscNames bool, args *NameArgs) string {
-	if args == nil || !args.Escape {
+	// TODO: Kill [NameArgs] and just pass [DestinationKind].
+	if args == nil {
 		return name
 	}
 

diff --git a/lib/sql/escape_test.go b/lib/sql/escape_test.go
@@ -24,106 +24,89 @@ func TestEscapeNameIfNecessary(t *testing.T) {
 			expectedNameWhenUpperCfg: "order",
 		},
 		{
-			name:                     "escape = false",
-			args:                     &NameArgs{},
-			nameToEscape:             "order",
-			expectedName:             "order",
-			expectedNameWhenUpperCfg: "order",
-		},
-		{
-			name: "escape = true, snowflake",
+			name: "snowflake",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Snowflake,
 			},
 			nameToEscape:             "order",
 			expectedName:             `"order"`,
 			expectedNameWhenUpperCfg: `"ORDER"`,
 		},
 		{
-			name: "escape = true, snowflake #2",
+			name: "snowflake #2",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Snowflake,
 			},
 			nameToEscape:             "hello",
 			expectedName:             `hello`,
 			expectedNameWhenUpperCfg: "hello",
 		},
 		{
-			name: "escape = true, redshift",
+			name: "redshift",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Redshift,
 			},
 			nameToEscape:             "order",
 			expectedName:             `"order"`,
 			expectedNameWhenUpperCfg: `"ORDER"`,
 		},
 		{
-			name: "escape = true, redshift #2",
+			name: "redshift #2",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Redshift,
 			},
 			nameToEscape:             "hello",
 			expectedName:             `hello`,
 			expectedNameWhenUpperCfg: "hello",
 		},
 		{
-			name: "escape = true, bigquery",
+			name: "bigquery",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.BigQuery,
 			},
 			nameToEscape:             "order",
 			expectedName:             "`order`",
 			expectedNameWhenUpperCfg: "`ORDER`",
 		},
 		{
-			name: "escape = true, bigquery, #2",
+			name: "bigquery, #2",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.BigQuery,
 			},
 			nameToEscape:             "hello",
 			expectedName:             "hello",
 			expectedNameWhenUpperCfg: "hello",
 		},
 		{
-			name: "escape = true, redshift, #1 (delta)",
+			name: "redshift, #1 (delta)",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Redshift,
 			},
 			nameToEscape:             "delta",
 			expectedName:             `"delta"`,
 			expectedNameWhenUpperCfg: `"DELTA"`,
 		},
 		{
-			name: "escape = true, snowflake, #1 (delta)",
+			name: "snowflake, #1 (delta)",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Snowflake,
 			},
 			nameToEscape:             "delta",
 			expectedName:             `delta`,
 			expectedNameWhenUpperCfg: `delta`,
 		},
 		{
-			name: "escape = true, redshift, symbols",
+			name: "redshift, symbols",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Redshift,
 			},
 			nameToEscape:             "receivedat:__",
 			expectedName:             `"receivedat:__"`,
 			expectedNameWhenUpperCfg: `"RECEIVEDAT:__"`,
 		},
 		{
-			name: "escape = true, redshift, numbers",
+			name: "redshift, numbers",
 			args: &NameArgs{
-				Escape:   true,
 				DestKind: constants.Redshift,
 			},
 			nameToEscape:             "0",

diff --git a/lib/typing/columns/columns.go b/lib/typing/columns/columns.go
@@ -253,7 +253,7 @@ func (c *Columns) UpdateQuery(destKind constants.DestinationKind, uppercaseEscNa
 			continue
 		}
 
-		colName := column.Name(uppercaseEscNames, &sql.NameArgs{Escape: true, DestKind: destKind})
+		colName := column.Name(uppercaseEscNames, &sql.NameArgs{DestKind: destKind})
 		if column.ToastColumn {
 			if column.KindDetails == typing.Struct {
 				cols = append(cols, processToastStructCol(colName, destKind))

diff --git a/lib/typing/columns/columns_test.go b/lib/typing/columns/columns_test.go
@@ -170,16 +170,11 @@ func TestColumn_Name(t *testing.T) {
 		}
 
 		assert.Equal(t, testCase.expectedName, col.RawName(), testCase.colName)
-		assert.Equal(t, testCase.expectedName, col.Name(false, &sql.NameArgs{
-			Escape: false,
-		}), testCase.colName)
 
 		assert.Equal(t, testCase.expectedNameEsc, col.Name(false, &sql.NameArgs{
-			Escape:   true,
 			DestKind: constants.Snowflake,
 		}), testCase.colName)
 		assert.Equal(t, testCase.expectedNameEscBq, col.Name(false, &sql.NameArgs{
-			Escape:   true,
 			DestKind: constants.BigQuery,
 		}), testCase.colName)
 	}
@@ -242,17 +237,12 @@ func TestColumns_GetColumnsToUpdate(t *testing.T) {
 		}
 
 		assert.Equal(t, testCase.expectedCols, cols.GetColumnsToUpdate(false, nil), testCase.name)
-		assert.Equal(t, testCase.expectedCols, cols.GetColumnsToUpdate(false, &sql.NameArgs{
-			Escape: false,
-		}), testCase.name)
 
 		assert.Equal(t, testCase.expectedColsEsc, cols.GetColumnsToUpdate(false, &sql.NameArgs{
-			Escape:   true,
 			DestKind: constants.Snowflake,
 		}), testCase.name)
 
 		assert.Equal(t, testCase.expectedColsEscBq, cols.GetColumnsToUpdate(false, &sql.NameArgs{
-			Escape:   true,
 			DestKind: constants.BigQuery,
 		}), testCase.name)
 	}

diff --git a/lib/typing/columns/wrapper_test.go b/lib/typing/columns/wrapper_test.go
@@ -44,7 +44,6 @@ func TestWrapper_Complete(t *testing.T) {
 	for _, testCase := range testCases {
 		// Snowflake escape
 		w := NewWrapper(NewColumn(testCase.name, typing.Invalid), false, &sql.NameArgs{
-			Escape:   true,
 			DestKind: constants.Snowflake,
 		})
 
@@ -53,7 +52,6 @@ func TestWrapper_Complete(t *testing.T) {
 
 		// BigQuery escape
 		w = NewWrapper(NewColumn(testCase.name, typing.Invalid), false, &sql.NameArgs{
-			Escape:   true,
 			DestKind: constants.BigQuery,
 		})
 
@@ -62,11 +60,8 @@ func TestWrapper_Complete(t *testing.T) {
 
 		for _, destKind := range []constants.DestinationKind{constants.Snowflake, constants.BigQuery} {
 			w = NewWrapper(NewColumn(testCase.name, typing.Invalid), false, &sql.NameArgs{
-				Escape:   false,
 				DestKind: destKind,
 			})
-
-			assert.Equal(t, testCase.expectedRawName, w.EscapedName(), testCase.name)
 			assert.Equal(t, testCase.expectedRawName, w.RawName(), testCase.name)
 		}