Skip to content

Some BigQuery cleanups#13812

Merged
hashhar merged 2 commits intomasterfrom
hashhar/bigquery-cleanups
Aug 24, 2022
Merged

Some BigQuery cleanups#13812
hashhar merged 2 commits intomasterfrom
hashhar/bigquery-cleanups

Conversation

@hashhar
Copy link
Copy Markdown
Member

@hashhar hashhar commented Aug 24, 2022

Addresses comments from #13094

@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label Aug 24, 2022
@hashhar hashhar requested a review from ebyhr August 24, 2022 05:59
@cla-bot cla-bot bot added the cla-signed label Aug 24, 2022
@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Aug 24, 2022

While working on this I was also trying to add tests which CREATE TABLE, CTAS and INSERT into a non-lowercase schema and found that CTAS doesn't work with case-insensitive matching for some connectors:

We cannot CTAS into non-lowercase schema in some connectors because the schema name of the output table handle (temporary table) doesn't match the schema name of the target table so it fails when calling commitCreateTable > renameTable complaining that connector doesn't support renaming tables across schemas.

I'm not going to fix it because I think it'd be a much better use of time to work on #17 instead (which I'm planning to pick up in my free time).

@hashhar hashhar merged commit 42f3b8a into master Aug 24, 2022
@hashhar hashhar deleted the hashhar/bigquery-cleanups branch August 24, 2022 08:17
@github-actions github-actions bot added this to the 394 milestone Aug 24, 2022
Comment on lines +232 to +235
String schemaName = "Test_Create_Case_Sensitive_" + randomTableSuffix();
assertUpdate("CREATE SCHEMA " + schemaName.toLowerCase(ENGLISH));
assertQuery(format("SELECT schema_name FROM information_schema.schemata WHERE schema_name = '%s'", schemaName.toLowerCase(ENGLISH)), format("VALUES '%s'", schemaName.toLowerCase(ENGLISH)));
assertUpdate("DROP SCHEMA " + schemaName.toLowerCase(ENGLISH));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@hashhar does this test have sense within CaseInsensitiveMapping class since all occurrences can be extracted
String lowerCase = schemaName.toLowerCase(ENGLISH);
and used through out the method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants