Skip to content
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.

Sign up for GitHub

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

[sql] Kill NameArgs.Escape #484

Merged
merged 3 commits into from
Apr 22, 2024
Merged

[sql] Kill NameArgs.Escape #484

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
536c40e
[sql] Kill `NameArgs.Escape`
nathan-artie Apr 22, 2024
fd7b0f8
Add back test
nathan-artie Apr 22, 2024
0fbb1a2
Add TODO
nathan-artie Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clients/bigquery/tableid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}),
)
}
2 changes: 1 addition & 1 deletion clients/mssql/tableid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}),
)
}
2 changes: 1 addition & 1 deletion clients/redshift/tableid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}),
)
}
2 changes: 1 addition & 1 deletion clients/shared/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
2 changes: 1 addition & 1 deletion clients/shared/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 0 additions & 1 deletion clients/snowflake/staging.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "%"))
Expand Down
2 changes: 1 addition & 1 deletion clients/snowflake/tableid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}),
)
}
2 changes: 0 additions & 2 deletions lib/destination/ddl/ddl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
})

Expand All @@ -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(),
}))
}
Expand Down
3 changes: 0 additions & 3 deletions lib/destination/ddl/ddl_bq_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions lib/destination/ddl/ddl_sflk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()})))
}
}

Expand Down
3 changes: 0 additions & 3 deletions lib/destination/dml/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ func (m *MergeArgument) GetParts() ([]string, error) {
}

cols := m.Columns.GetColumnsToUpdate(*m.UppercaseEscNames, &sql.NameArgs{
Escape: true,
DestKind: m.DestKind,
})

Expand Down Expand Up @@ -234,7 +233,6 @@ func (m *MergeArgument) GetStatement() (string, error) {
}

cols := m.Columns.GetColumnsToUpdate(*m.UppercaseEscNames, &sql.NameArgs{
Escape: true,
DestKind: m.DestKind,
})

Expand Down Expand Up @@ -306,7 +304,6 @@ func (m *MergeArgument) GetMSSQLStatement() (string, error) {
}

cols := m.Columns.GetColumnsToUpdate(*m.UppercaseEscNames, &sql.NameArgs{
Escape: true,
DestKind: m.DestKind,
})

Expand Down
2 changes: 0 additions & 2 deletions lib/destination/dml/merge_parts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}))
}
Expand Down
2 changes: 0 additions & 2 deletions lib/destination/dml/merge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
},
Expand Down
3 changes: 1 addition & 2 deletions lib/sql/escape.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe? What about callers that expect the column to not be escaped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't any callers that expect this column not to be escaped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args.Escape is always true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to kill NameArgs and just pass DestKind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, and that's what I originally wanted to do, but it's a little tricky because sometimes args is null and it's annoying to create a pointer to DestinationKind. Maybe I'll do that anyways since it's a bit safer.

Copy link
Contributor Author

@nathan-artie nathan-artie Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried again and it ends up being a bigger lift than I want to do right now, will look into it some other time.

if args == nil {
return name
}

Expand Down
37 changes: 10 additions & 27 deletions lib/sql/escape_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion lib/typing/columns/columns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
10 changes: 0 additions & 10 deletions lib/typing/columns/columns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
5 changes: 0 additions & 5 deletions lib/typing/columns/wrapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})

Expand All @@ -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,
})

Expand All @@ -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)
}

Expand Down