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.

Sign up for GitHub

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

Jump to bottom

destination-redshift: add option for drop cascade #38189

Merged
merged 1 commit into from
May 14, 2024
Merged

destination-redshift: add option for drop cascade #38189

merged 1 commit into from
May 14, 2024

Conversation

stephane-airbyte
Copy link
Contributor

@stephane-airbyte stephane-airbyte commented May 14, 2024 •
edited by evantahler
Loading

@vercel Vercel
Copy link

vercel bot commented May 14, 2024 •
edited
Loading

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 May 14, 2024 6:22pm

@stephane-airbyte Graphite App
Copy link
Contributor Author

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

Join @stephane-airbyte and the rest of your teammates on Graphite Graphite

@stephane-airbyte stephane-airbyte marked this pull request as ready for review May 14, 2024 16:12
@stephane-airbyte stephane-airbyte requested a review from a team as a code owner May 14, 2024 16:12
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch 2 times, most recently from adfc65f to f4997f3 Compare May 14, 2024 17:02
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch from f4997f3 to 1164db0 Compare May 14, 2024 17:04
@stephane-airbyte stephane-airbyte requested review from edgao and gisripa May 14, 2024 17:04
edgao
edgao approved these changes May 14, 2024
View reviewed changes
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.

nit: pr title should be redshift, and I think you linked the wrong issue in the description?

otherwise, one comment about structure, lgtm assuming CI succeeds

...src/main/java/io/airbyte/integrations/destination/redshift/RedshiftStagingS3Destination.java Outdated
@@ -179,7 +180,10 @@ public JsonNode toJdbcConfig(final JsonNode config) {

@Override
protected JdbcSqlGenerator getSqlGenerator(final JsonNode config) {
return new RedshiftSqlGenerator(getNamingResolver());

final JsonNode dropCascadeNode = config.get(RedshiftDestinationConstants.DROP_CASCADE_OPTION);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we shove this into a shared method somewhere? (to avoid duplicating between staging+insert destinations)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@stephane-airbyte stephane-airbyte changed the title destination-bigquery: add option for drop cascade destination-redshift: add option for drop cascade May 14, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch from 1164db0 to e595dc7 Compare May 14, 2024 17:06
@octavia-squidington-iii octavia-squidington-iii removed the area/documentation Improvements or additions to documentation label May 14, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch from e595dc7 to 2aec0d1 Compare May 14, 2024 17:07
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label May 14, 2024
evantahler
evantahler reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

This is great - it gives users the option to self-solve... but:

  1. we still need to catch the error thrown in this case and turn it into a config_error, not a system_error
  2. I would have expected to see the spec change to include the new option?

@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch from 2aec0d1 to 9737d0e Compare May 14, 2024 17:13
@stephane-airbyte Graphite App
Copy link
Contributor Author

@evan: indeed. My PRs got mixed up on my local...

@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch from 9737d0e to ed994c5 Compare May 14, 2024 17:17
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch from ed994c5 to 88f9755 Compare May 14, 2024 17:44
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch from 88f9755 to cfefcf0 Compare May 14, 2024 18:18
evantahler
evantahler reviewed May 14, 2024
View reviewed changes
...io/airbyte/integrations/destination/redshift/typing_deduping/RedshiftDestinationHandler.java
Comment on lines +55 to +58
if (e.getMessage().contains("ERROR: cannot drop table") && e.getMessage().contains("because other objects depend on it")) {
throw new ConfigErrorException(
"Failed to drop table without the CASCADE option. Consider changing the drop_cascade configuration parameter", e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly the type of 'translation' we'll want to move into a more formal 'usability layer' error handling class soon

@stephane-airbyte stephane-airbyte merged commit 47f304b into master May 14, 2024
36 checks passed
@stephane-airbyte Graphite App
Copy link
Contributor Author

Merge activity

@stephane-airbyte stephane-airbyte deleted the stephane/05-14-destination-bigquery_add_option_for_drop_cascade branch May 14, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgao edgao edgao approved these changes

@evantahler evantahler evantahler approved these changes

@gisripa gisripa Awaiting requested review from gisripa

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/destination/bigquery connectors/destination/redshift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stephane-airbyte @evantahler @edgao @octavia-squidington-iii