-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
...otlin/io/airbyte/integrations/destination/snowflake/typing_deduping/SnowflakeSqlGenerator.kt
Outdated
Show resolved
Hide resolved
...otlin/io/airbyte/integrations/destination/snowflake/typing_deduping/SnowflakeSqlGenerator.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
There was a problem hiding this 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
...otlin/io/airbyte/integrations/destination/snowflake/typing_deduping/SnowflakeSqlGenerator.kt
Outdated
Show resolved
Hide resolved
368fd1d
to
efc745e
Compare
efc745e
to
a18933f
Compare
What
Cleaning up
SnowflakeSqlGenerator.kt
by replacingapache.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 theapache.commons.text
dependency in gradle yet since it is still used inSnowflakeDestinationHandler.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 idiomsUser Impact
None. Zero. Zilch.
Can this PR be safely reverted and rolled back?