-
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: cleaning up unused code paths #38553
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
a3d34bc
to
217c373
Compare
...lake/src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeSqlOperations.java
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.
tiny nits. I only skimmed SnowflakeDestination, since I assume that's mostly just autoconverted code?
// verify if user has permission to make SQL INSERT queries | ||
try { | ||
if (attemptInsert) { | ||
sqlOps.insertRecords( |
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.
sigh. snowflake shouldn't even be using this method. fine for now, but eventually we should have snowflake do the actual internal stage + COPY thing for check
...lake/src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeSqlOperations.java
Outdated
Show resolved
Hide resolved
@@ -361,4 +362,9 @@ private String toJdbcTypeName(final AirbyteProtocolType airbyteProtocolType) { | |||
}; | |||
} | |||
|
|||
@Override | |||
public void createNamespaces(@NotNull Set<String> schemas) { | |||
|
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.
nit: is it safe to throw new UnsupportedOperationException("not yet implemented")
? (just to be explicit that this should be implemented, we just haven't gotten around to it yet
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.
yeah was planning to add that. just to be sure it doesn't get called erroneously.
...wflake/src/main/kotlin/io/airbyte/integrations/destination/snowflake/SnowflakeDestination.kt
Outdated
Show resolved
Hide resolved
...ke/src/test/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestinationTest.java
Show resolved
Hide resolved
e67a86e
to
41fd326
Compare
41fd326
to
ba3d8fc
Compare
71b6cba
to
3ac8c86
Compare
ba3d8fc
to
e3601d8
Compare
e3601d8
to
1eb280e
Compare
1eb280e
to
429ce3a
Compare
What
Remove remnants of
SwitchingDestination
and decouple Snowflake fromAbstractJdbcDestination
base implementation (nothing is reused except a check related function)User Impact
Can this PR be safely reverted and rolled back?