🐛 Destination snowflake: Create final tables with uppercase naming #30056

edgao · 2023-08-31T20:02:56Z

The interesting changes are in SnowflakeDestinationHandler / SnowflakeSqlGenerator; everything else is just updating tests. (T+D tests are passing locally, at least)

See also #30068, which implements the migration for existing tables. I'll merge that PR into this one, then release this PR to master.

todo:

~~add migration logic. I'll stack a PR on top of this one.~~ Destination snowflake: table casing migration #30068
~~run some manual tests~~
~~update tests to have uppercase _airbyte_* in the expected records. I'll submit an issue to do that - the columns are uppercased in the destination, just the expected records look weird.~~ TypingAndDedupingTests: RecordDiffer should accept airbyte metadata column names instead of hardcoding them #30112

github-actions · 2023-08-31T20:03:17Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

evantahler

There's a lot of toUpperCase() in the implementation code. I thought we wanted to let the DB evaluate these these to UPPERCASE lazily?

evantahler · 2023-08-31T21:45:54Z

.../test-integration/java/io/airbyte/integrations/destination/snowflake/SnowflakeTestUtils.java

+        unescapeIdentifier(databaseName).toUpperCase(),
+        unescapeIdentifier(schema).toUpperCase(),
+        unescapeIdentifier(table).toUpperCase()).stream()


This is a test util, so maybe it doens't matter, but I thought we wanted to let the DB evaluate the upper-case-ness of table names

I overruled that decision https://airbytehq-team.slack.com/archives/C03C4AVJWG4/p1693495091006359 (but open to continued discussion!)

Yes!

with snowflake uppercasing via not-quoting, we’re losing the ability to use special characters / reserved words in namespace/table/column names. That opens up a whole can of worms around escaping things, which I’d rather not deal with right now.

That makes a lot of sense. 👍

this is still technically a user-facing decision (e.g. would they rather get "FOO$BAR" or FOO_BAR). But that's a much smaller blast radius

Co-authored-by: edgao <[email protected]>

edgao · 2023-09-01T20:39:48Z

docs/release_notes/upgrading_to_destinations_v2.md

@@ -175,12 +175,6 @@ In addition to the changes which apply for all destinations described above, the
 1. [Object and array properties](https://docs.airbyte.com/understanding-airbyte/supported-data-types/#the-types) are properly stored as JSON columns. Previously, we had used TEXT, which made querying sub-properties more difficult.
   - In certain cases, numbers within sub-properties with long decimal values will need to be converted to float representations due to a _quirk_ of Bigquery. Learn more [here](https://github.com/airbytehq/airbyte/issues/29594).

-### Snowflake
-
-1. `destination-snowflake` is now case-sensitive, and was not previously. This means that if you have a source stream "users", `destination-snowflake` would have previously created a "USERS" table in your data warehouse. We now correctly create a "users" table.


@Hesperide fyi - I'm deleting the case-sensitivity thing from snowflake's upgrade guide.

github-actions · 2023-09-05T15:18:26Z

destination-bigquery test report (commit `d8c27a2e81`) - ✅

⏲️ Total pipeline duration: 10mn44s

Step	Result
Java Connector Unit Tests	✅
Build connector tar	✅
Build destination-bigquery docker image for platform linux/x86_64	✅
Java Connector Integration Tests	✅
Validate airbyte-integrations/connectors/destination-bigquery/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-bigquery test

github-actions · 2023-09-05T15:21:54Z

destination-snowflake test report (commit `d8c27a2e81`) - ✅

⏲️ Total pipeline duration: 03mn13s

Step	Result
Java Connector Unit Tests	✅
Build connector tar	✅
Build destination-snowflake docker image for platform linux/x86_64	✅
Java Connector Integration Tests	✅
Validate airbyte-integrations/connectors/destination-snowflake/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-snowflake test

edgao · 2023-09-05T16:03:35Z

source-hubplanner is an unrelated autoformat change. Will approve-and-merge this PR.

edgao · 2023-09-05T16:54:10Z

/approve-and-merge reason="failing test is due to unrelated connector; test only ran because of autoformat. bigquery+snowflake have green CI"

octavia-approvington · 2023-09-05T16:54:50Z

Send it!

octavia-squidington-iii added area/connectors Connector related issues connectors/destination/bigquery connectors/destination/snowflake labels Aug 31, 2023

octavia-squidington-iii added the connectors/source/clockify label Aug 31, 2023

edgao and others added 13 commits August 31, 2023 13:26

just use generator.createTable

253d8b3

remove unrelated change

5380a7d

add ability to customize stream id

3f2d1d4

wip upcase final table stuff

d5c3f4c

progress on making tests pass

8a17f26

more fixes

c0e1e37

fix createTable test

dbbfd26

more fix

c4c362c

more fix

75663db

more fix

09ea373

Automated Commit - Formatting Changes

b55d80d

make DATs pass

3591162

resolve todo

befa893

edgao force-pushed the edgao/dv2/snowflake/case_sensitivity branch from b226c19 to befa893 Compare August 31, 2023 20:26

edgao marked this pull request as ready for review August 31, 2023 21:26

edgao requested a review from a team as a code owner August 31, 2023 21:26

This comment was marked as outdated.

Sign in to view

evantahler reviewed Aug 31, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

edgao mentioned this pull request Aug 31, 2023

Destination snowflake: table casing migration #30068

Merged

jbfbell approved these changes Sep 1, 2023

View reviewed changes

edgao mentioned this pull request Sep 1, 2023

TypingAndDedupingTests: RecordDiffer should accept airbyte metadata column names instead of hardcoding them #30112

Closed

edgao and others added 3 commits September 1, 2023 13:35

Destination snowflake: table casing migration (#30068)

abde8fa

Co-authored-by: edgao <[email protected]>

Merge branch 'master' into edgao/dv2/snowflake/case_sensitivity

43b2412

logistics

6d71b7d

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Sep 1, 2023

delete case-sensitivity from upgrade guide

b31863d

edgao commented Sep 1, 2023

View reviewed changes

edgao changed the title ~~Destination snowflake: Create final tables with uppercase naming~~ 🐛 Destination snowflake: Create final tables with uppercase naming Sep 1, 2023

edgao added the checklist-action-run label Sep 1, 2023

This comment was marked as outdated.

Sign in to view

also bigquery logistics

06277dd

This comment was marked as outdated.

Sign in to view

edgao and others added 3 commits September 5, 2023 07:33

Merge branch 'master' into edgao/dv2/snowflake/case_sensitivity

33bcd9e

merge pls

26e654f

Automated Commit - Formatting Changes

c59f4eb

This comment was marked as outdated.

Sign in to view

Merge branch 'master' into edgao/dv2/snowflake/case_sensitivity

d8c27a2

octavia-squidington-iii added the connectors/source/hubplanner label Sep 5, 2023

This comment was marked as outdated.

Sign in to view

This comment was marked as off-topic.

Sign in to view

octavia-approvington approved these changes Sep 5, 2023

View reviewed changes

octavia-approvington merged commit 9231c04 into master Sep 5, 2023

octavia-approvington deleted the edgao/dv2/snowflake/case_sensitivity branch September 5, 2023 16:54

evantahler mentioned this pull request Sep 5, 2023

[Roadmap] Airbyte Destinations V2: Improved Data Syncing & Error Handling #26028

Closed

rtol5 mentioned this pull request Sep 5, 2023

Upgrading destination-snowflake to Destinations V2 loses case insensitivity with no warning or ability to preserve case insensitivity, breaking all query paths. #30139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Destination snowflake: Create final tables with uppercase naming #30056

🐛 Destination snowflake: Create final tables with uppercase naming #30056

edgao commented Aug 31, 2023 •

edited

Loading

github-actions bot commented Aug 31, 2023 •

edited by edgao

Loading

This comment was marked as outdated.

evantahler left a comment

evantahler Aug 31, 2023

edgao Aug 31, 2023

evantahler Aug 31, 2023

edgao Aug 31, 2023

This comment was marked as outdated.

edgao Sep 1, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as off-topic.

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

edgao commented Sep 5, 2023

edgao commented Sep 5, 2023

octavia-approvington commented Sep 5, 2023

🐛 Destination snowflake: Create final tables with uppercase naming #30056

🐛 Destination snowflake: Create final tables with uppercase naming #30056

Conversation

edgao commented Aug 31, 2023 • edited Loading

github-actions bot commented Aug 31, 2023 • edited by edgao Loading

Before Merging a Connector Pull Request

This comment was marked as outdated.

evantahler left a comment

Choose a reason for hiding this comment

evantahler Aug 31, 2023

Choose a reason for hiding this comment

edgao Aug 31, 2023

Choose a reason for hiding this comment

evantahler Aug 31, 2023

Choose a reason for hiding this comment

edgao Aug 31, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

edgao Sep 1, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as off-topic.

github-actions bot commented Sep 5, 2023

destination-bigquery test report (commit d8c27a2e81) - ✅

github-actions bot commented Sep 5, 2023

destination-snowflake test report (commit d8c27a2e81) - ✅

edgao commented Sep 5, 2023

edgao commented Sep 5, 2023

octavia-approvington commented Sep 5, 2023

edgao commented Aug 31, 2023 •

edited

Loading

github-actions bot commented Aug 31, 2023 •

edited by edgao

Loading

destination-bigquery test report (commit `d8c27a2e81`) - ✅

destination-snowflake test report (commit `d8c27a2e81`) - ✅