Allow in-place column drop for bigquery table

[obada-ab]

Partial fix for hashicorp/terraform-provider-google#14769

Column deletion reference: https://cloud.google.com/bigquery/docs/managing-table-schemas#delete_a_column

Release Note Template for Downstream PRs (will be copied)

bigquery: added in-place column drop support for `bigquery_table`

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@hao-nan-li, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[hao-nan-li]

/gcbrun

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 175 insertions(+), 19 deletions(-))
google-beta provider: Diff ( 3 files changed, 175 insertions(+), 19 deletions(-))

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 175 insertions(+), 19 deletions(-))
google-beta provider: Diff ( 3 files changed, 175 insertions(+), 19 deletions(-))

[modular-magician]

Tests analytics

Total tests: 113
Passed tests: 86
Skipped tests: 11
Affected tests: 16

Click here to see the affected service packages

• bigquery

Action taken

Found 16 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigQueryDataTable_expandArray|TestAccBigQueryDataTable_jsonEquivalency|TestAccBigQueryExternalDataTable_CSV|TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionID_UpdateNoConnectionID|TestAccBigQueryExternalDataTable_CSV_WithSchema_UpdateToConnectionID|TestAccBigQueryExternalDataTable_parquetOptions|TestAccBigQueryTable_DropColumn|TestAccBigQueryTable_MaterializedView_DailyTimePartioning_Update|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTag|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTagNames|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToNoPolicyTag|TestAccBigQueryTable_Update_SchemaWithoutPolicyTagsToWithPolicyTags|TestAccBigQueryTable_WithViewAndSchema|TestAccBigQueryTable_allowDestroy|TestAccBigQueryTable_updateTableConstraints|TestAccBigQueryTable_updateView

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_expandArray[Debug log]
TestAccBigQueryDataTable_jsonEquivalency[Debug log]
TestAccBigQueryExternalDataTable_CSV[Debug log]
TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionID_UpdateNoConnectionID[Debug log]
TestAccBigQueryExternalDataTable_CSV_WithSchema_UpdateToConnectionID[Debug log]
TestAccBigQueryExternalDataTable_parquetOptions[Debug log]
TestAccBigQueryTable_DropColumn[Debug log]
TestAccBigQueryTable_MaterializedView_DailyTimePartioning_Update[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTag[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTagNames[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToNoPolicyTag[Debug log]
TestAccBigQueryTable_Update_SchemaWithoutPolicyTagsToWithPolicyTags[Debug log]
TestAccBigQueryTable_WithViewAndSchema[Debug log]
TestAccBigQueryTable_allowDestroy[Debug log]
TestAccBigQueryTable_updateTableConstraints[Debug log]
TestAccBigQueryTable_updateView[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[modular-magician]

Tests analytics

Total tests: 113
Passed tests: 86
Skipped tests: 11
Affected tests: 16

Click here to see the affected service packages

• bigquery

Action taken

Found 16 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigQueryDataTable_expandArray|TestAccBigQueryDataTable_jsonEquivalency|TestAccBigQueryExternalDataTable_CSV|TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionID_UpdateNoConnectionID|TestAccBigQueryExternalDataTable_CSV_WithSchema_UpdateToConnectionID|TestAccBigQueryExternalDataTable_parquetOptions|TestAccBigQueryTable_DropColumn|TestAccBigQueryTable_MaterializedView_DailyTimePartioning_Update|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTag|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTagNames|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToNoPolicyTag|TestAccBigQueryTable_Update_SchemaWithoutPolicyTagsToWithPolicyTags|TestAccBigQueryTable_WithViewAndSchema|TestAccBigQueryTable_allowDestroy|TestAccBigQueryTable_updateTableConstraints|TestAccBigQueryTable_updateView

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_expandArray[Debug log]
TestAccBigQueryDataTable_jsonEquivalency[Debug log]
TestAccBigQueryExternalDataTable_CSV[Debug log]
TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionID_UpdateNoConnectionID[Debug log]
TestAccBigQueryExternalDataTable_CSV_WithSchema_UpdateToConnectionID[Debug log]
TestAccBigQueryExternalDataTable_parquetOptions[Debug log]
TestAccBigQueryTable_DropColumn[Debug log]
TestAccBigQueryTable_MaterializedView_DailyTimePartioning_Update[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTag[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTagNames[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToNoPolicyTag[Debug log]
TestAccBigQueryTable_Update_SchemaWithoutPolicyTagsToWithPolicyTags[Debug log]
TestAccBigQueryTable_WithViewAndSchema[Debug log]
TestAccBigQueryTable_allowDestroy[Debug log]
TestAccBigQueryTable_updateTableConstraints[Debug log]
TestAccBigQueryTable_updateView[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[hao-nan-li]

Could you briefly explain why this change is necessary?

[wj-chen]

Do you think this logic would be more self-descriptive if we also tracked
e.g. "sameColumns" as we loop through the keys and just check old map size against the sum here instead?

[obada-ab]

I think "changeableColumns" would be more precise since changing a column's mode would be allowed alongside column drops, let me know if you think we should change this to only column drops without other in-place changes (besides adding columns)

and just check old map size against the sum here instead?

hmm not sure if I get it, if the condition is something like

(droppedColumns == 0) || (len(arrayOld) == changeableColumns+droppedColumns)

then it wouldn't work correctly, for example when changing {col1, col2, col3} to {col1, col4, col5} the condition would result in true (changeableColumns=1, droppedColumns=2)

I think we can do something like:

newColumns := len(arrayNew) - changeableColumns
return (droppedColumns == 0) || (newColumns == 0), nil

what do you think?

[wj-chen]

Let's still include other in-place changes. Naming is hard though - the columns would have the same name but maybe different other attributes. "SameNameColumns"?

Your updated logic looks good.

[obada-ab]

Could you briefly explain why this change is necessary?

@hao-nan-li many customers are having a hard time managing BigQuery tables when applying some types of changes to the schema field that can happen in-place, including column drops which are addressed in this PR. The current behavior of the BigQuery table resource is to force-new when a column is dropped, which causes all existing table data to be removed.

More info: hashicorp/terraform-provider-google#14769 b/298496481

[hao-nan-li]

/gcbrun

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 196 insertions(+), 22 deletions(-))
google-beta provider: Diff ( 3 files changed, 196 insertions(+), 22 deletions(-))

[modular-magician]

Tests analytics

Total tests: 113
Passed tests: 98
Skipped tests: 11
Affected tests: 4

Click here to see the affected service packages

• bigquery

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigQueryTable_DropColumns|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTag|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTagNames|TestAccBigQueryTable_Update_SchemaWithPolicyTagsToNoPolicyTag

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryTable_DropColumns[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTag[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToEmptyPolicyTagNames[Debug log]
TestAccBigQueryTable_Update_SchemaWithPolicyTagsToNoPolicyTag[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[wj-chen]

Let's still include other in-place changes. Naming is hard though - the columns would have the same name but maybe different other attributes. "SameNameColumns"?

Your updated logic looks good.

[hao-nan-li]

/gcbrun

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 198 insertions(+), 17 deletions(-))
google-beta provider: Diff ( 3 files changed, 198 insertions(+), 17 deletions(-))

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 198 insertions(+), 17 deletions(-))
google-beta provider: Diff ( 3 files changed, 198 insertions(+), 17 deletions(-))

[modular-magician]

Tests analytics

Total tests: 113
Passed tests: 102
Skipped tests: 11
Affected tests: 0

Click here to see the affected service packages

• bigquery

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[modular-magician]

Tests analytics

Total tests: 113
Passed tests: 102
Skipped tests: 11
Affected tests: 0

Click here to see the affected service packages

• bigquery

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[modular-magician]

Tests analytics

Total tests: 114
Passed tests: 103
Skipped tests: 11
Affected tests: 0

Click here to see the affected service packages

• bigquery

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[wj-chen]

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?

[obada-ab]

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.

[wj-chen]

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.

[obada-ab]

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.

[wj-chen]

The new behavior is problematic for external tables, I tested the configuration in hashicorp/terraform-provider-google#12465 (comment), and I got an error when performing terraform apply the second time:

ALTER TABLE only supports altering schema for tables while my_table is a EXTERNAL TABLE

Made changes so that the new behavior doesn't apply for external tables, tested the external table configuration again and now the provider forces replacement.

Thank you for double checking and making the corresponding change!

[wj-chen]

/gcbrun

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 222 insertions(+), 22 deletions(-))
google-beta provider: Diff ( 3 files changed, 222 insertions(+), 22 deletions(-))

[modular-magician]

Tests analytics

Total tests: 114
Passed tests: 103
Skipped tests: 11
Affected tests: 0

Click here to see the affected service packages

• bigquery

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[obada-ab]

@hao-nan-li can you take a look