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

Allow in-place column drop for bigquery table #10170

Merged
merged 10 commits into from
Apr 4, 2024
Merged

Allow in-place column drop for bigquery table #10170

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e5f2d41
Allow in-place column drop for bigquery table
obada-ab Mar 12, 2024
c6f1e95
Allow only pure in-place column drops
obada-ab Mar 21, 2024
e8790e2
Merge branch 'GoogleCloudPlatform:main' into schema-update
obada-ab Mar 21, 2024
3e84fac
Fix column drop logic
obada-ab Mar 22, 2024
f58baf1
Merge branch 'GoogleCloudPlatform:main' into schema-update
obada-ab Mar 22, 2024
4c19ade
Fix schemaIsChangeableNested test cases
obada-ab Mar 25, 2024
6f41b8e
Fix dropping columns for external tables
obada-ab Mar 27, 2024
68f9b14
Refactor nested schema changable test
obada-ab Apr 2, 2024
93ca7f5
Refactor table schema changeable logic
obada-ab Apr 3, 2024
4c5d641
Merge branch 'GoogleCloudPlatform:main' into schema-update
obada-ab Apr 3, 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
21 changes: 13 additions & 8 deletions mmv1/third_party/terraform/services/bigquery/resource_bigquery_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, topLevel bool
case []interface{}:
arrayOld := old.([]interface{})
arrayNew, ok := new.([]interface{})
sameNameColumns := 0
droppedColumns := 0
if !ok {
// if not both arrays not changeable
Expand Down Expand Up @@ -298,9 +299,13 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, topLevel bool
resourceBigQueryTableSchemaIsChangeable(mapOld[key], mapNew[key], false); err != nil || !isChangable {
return false, err
}
Copy link
Member

Choose a reason for hiding this comment

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

I thought we'd be updating this as we look through mapOld and comparing key to mapNew - could you explain how this is working as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I understand the question correctly, we are going through the keys in mapOld and comparing to mapNew, but we're doing it recursively because of the nature of the schema (nested interfaces). We immediately return false (not changeable) if we encounter an unchangeable case, for example a column that had it's type changed.

Copy link
Member

Choose a reason for hiding this comment

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

I was referring to this block:

if topLevel {
  sameNameColumns += 1
}

At a glance, in a single given level, top or nested, sameNameColumns will be 0 or 1. Additional comments on the variables and the logic here would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sameNameColumns is always 0 for any nested level, and could be 0 or more at the top level. I refactored this part of the code, it should still be the same logic but I think It's more readable now and less prone to accidental logic-breaking changes.

if topLevel {
sameNameColumns += 1
}
}
// only pure column drops allowed
return (droppedColumns == 0) || (len(arrayOld) == len(arrayNew)+droppedColumns), nil
newColumns := len(arrayNew) - sameNameColumns
return (droppedColumns == 0) || (newColumns == 0), nil
case map[string]interface{}:
objectOld := old.(map[string]interface{})
objectNew, ok := new.(map[string]interface{})
Expand Down Expand Up @@ -1705,7 +1710,7 @@ func resourceBigQueryTableRead(d *schema.ResourceData, meta interface{}) error {
return nil
}

type TableInfo struct {
type TableReference struct {
project string
datasetID string
tableID string
Expand Down Expand Up @@ -1733,13 +1738,13 @@ func resourceBigQueryTableUpdate(d *schema.ResourceData, meta interface{}) error
datasetID := d.Get("dataset_id").(string)
tableID := d.Get("table_id").(string)

tableInfo := &TableInfo{
tableReference := &TableReference{
project: project,
datasetID: datasetID,
tableID: tableID,
}

if err = resourceBigQueryTableColumnDrop(config, userAgent, table, tableInfo); err != nil {
if err = resourceBigQueryTableColumnDrop(config, userAgent, table, tableReference); err != nil {
return err
}

Expand All @@ -1750,8 +1755,8 @@ func resourceBigQueryTableUpdate(d *schema.ResourceData, meta interface{}) error
return resourceBigQueryTableRead(d, meta)
}

func resourceBigQueryTableColumnDrop(config *transport_tpg.Config, userAgent string, table *bigquery.Table, tableInfo *TableInfo) error {
oldTable, err := config.NewBigQueryClient(userAgent).Tables.Get(tableInfo.project, tableInfo.datasetID, tableInfo.tableID).Do()
func resourceBigQueryTableColumnDrop(config *transport_tpg.Config, userAgent string, table *bigquery.Table, tableReference *TableReference) error {
oldTable, err := config.NewBigQueryClient(userAgent).Tables.Get(tableReference.project, tableReference.datasetID, tableReference.tableID).Do()
if err != nil {
return err
}
Expand All @@ -1771,7 +1776,7 @@ func resourceBigQueryTableColumnDrop(config *transport_tpg.Config, userAgent str
if len(droppedColumns) > 0 {
droppedColumnsString := strings.Join(droppedColumns, ", DROP COLUMN ")

dropColumnsDDL := fmt.Sprintf("ALTER TABLE `%s.%s.%s` DROP COLUMN %s", tableInfo.project, tableInfo.datasetID, tableInfo.tableID, droppedColumnsString)
dropColumnsDDL := fmt.Sprintf("ALTER TABLE `%s.%s.%s` DROP COLUMN %s", tableReference.project, tableReference.datasetID, tableReference.tableID, droppedColumnsString)
log.Printf("[INFO] Dropping columns in-place: %s", dropColumnsDDL)

useLegacySQL := false
Expand All @@ -1780,7 +1785,7 @@ func resourceBigQueryTableColumnDrop(config *transport_tpg.Config, userAgent str
UseLegacySql: &useLegacySQL,
}

_, err = config.NewBigQueryClient(userAgent).Jobs.Query(tableInfo.project, req).Do()
_, err = config.NewBigQueryClient(userAgent).Jobs.Query(tableReference.project, req).Do()
if err != nil {
return err
}
Expand Down
20 changes: 11 additions & 9 deletions mmv1/third_party/terraform/services/bigquery/resource_bigquery_table_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,16 +576,18 @@ func TestUnitBigQueryDataTable_schemaIsChangeable(t *testing.T) {
func TestUnitBigQueryDataTable_schemaIsChangeableNested(t *testing.T) {
t.Parallel()
// Only top level column drops are changeable
skipNested := map[string]bool{"arraySizeDecreases": true, "typeModeReqToNullAndColumnDropped": true}
customNestedValues := map[string]bool{"arraySizeDecreases": false, "typeModeReqToNullAndColumnDropped": false}
for _, testcase := range testUnitBigQueryDataTableIsChangeableTestCases {
if !skipNested[testcase.name] {
testcaseNested := &testUnitBigQueryDataTableJSONChangeableTestCase{
testcase.name + "Nested",
fmt.Sprintf("[{\"name\": \"someValue\", \"type\" : \"INTEGER\", \"fields\" : %s }]", testcase.jsonOld),
fmt.Sprintf("[{\"name\": \"someValue\", \"type\" : \"INT64\", \"fields\" : %s }]", testcase.jsonNew),
testcase.changeable,
}
testcaseNested.check(t)
changeable, ok := customNestedValues[testcase.name]
if !ok {
changeable = testcase.changeable
}
testcaseNested := &testUnitBigQueryDataTableJSONChangeableTestCase{
testcase.name + "Nested",
fmt.Sprintf("[{\"name\": \"someValue\", \"type\" : \"INTEGER\", \"fields\" : %s }]", testcase.jsonOld),
fmt.Sprintf("[{\"name\": \"someValue\", \"type\" : \"INT64\", \"fields\" : %s }]", testcase.jsonNew),
changeable,
}
testcaseNested.check(t)
}
}