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.

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

Destination Snowflake: SqlGenerator class cleanup #43348

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

gisripa
Copy link
Contributor

@gisripa gisripa commented Aug 7, 2024

What

Cleaning up SnowflakeSqlGenerator.kt by replacing apache.commons.text.StringSubstitutor with Kotlin's native String interpolation. There is a lot of duplication of code snippets but did not touch it in this PR to minimize diffs. Cannot remove the apache.commons.text dependency in gradle yet since it is still used in SnowflakeDestinationHandler.kt

How

Review guide

No functional or query changes. Any diffs should be solely nuking StringSubtitutor references and replacing with Kotlin's string interpolation and simplifying String concatenation from collections into Kotlin idioms

User Impact

None. Zero. Zilch.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Aug 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2024 6:34pm

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Aug 7, 2024
Copy link
Contributor Author

gisripa commented Aug 7, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @gisripa and the rest of your teammates on Graphite Graphite

@gisripa gisripa marked this pull request as ready for review August 7, 2024 05:17
@gisripa gisripa requested a review from a team as a code owner August 7, 2024 05:17
@gisripa gisripa changed the title snowflake-sqlgen-cleanup Destination Snowflake: SqlGenerator class cleanup Aug 7, 2024
Copy link
Contributor

@stephane-airbyte stephane-airbyte left a comment

Choose a reason for hiding this comment

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

minor comments. No need to re-submit for review

Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

I only skimmed this but it looks generally right + the sqlgenerator tests should protect us. one nit about avoiding \", otherwise lgtm

@gisripa gisripa force-pushed the gireesh/08-06-snowflake-sqlgen-cleanup branch from 368fd1d to efc745e Compare August 7, 2024 17:41
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Aug 7, 2024
@gisripa gisripa force-pushed the gireesh/08-06-snowflake-sqlgen-cleanup branch from efc745e to a18933f Compare August 7, 2024 18:29
@gisripa gisripa merged commit 7cab6a2 into master Aug 7, 2024
38 checks passed
@gisripa gisripa deleted the gireesh/08-06-snowflake-sqlgen-cleanup branch August 7, 2024 19:10
LouisAuneau pushed a commit to LouisAuneau/airbyte that referenced this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/destination/snowflake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants