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

Namespace destination tables #1992

Closed

Conversation

ChristopheDuong
Copy link
Contributor

@ChristopheDuong ChristopheDuong commented Feb 8, 2021

What

Describe what the change is solving
Following-up on #1993, implements in destinations by using the new namespace fields

Implements #1921

How

Describe the solution

  • Refactor the NamingTransformer to split into
    • a NamingHelper class: where we specify prefix/suffix to table/schema names
    • NamingTransformer classes: only handles identifier specificities to each destinations only
  • Destination read and use namespace field in ConfiguredCatalog to specify where they should write the final table.

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

Refactoring naming utils classes to handle schema+table names:

  1. airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/destination/NamingConventionTransformer.java
  2. airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/destination/NamingHelper.java
  3. airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/destination/StandardNameTransformer.java

Adapting destinations to new naming:

  1. airbyte-integrations/connectors/destination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/JdbcBufferedConsumerFactory.java
  2. airbyte-integrations/connectors/destination-bigquery/src/main/java/io/airbyte/integrations/destination/bigquery/BigQueryDestination.java
  3. airbyte-integrations/connectors/destination-csv/src/main/java/io/airbyte/integrations/destination/csv/CsvDestination.java
  4. the rest

@ChristopheDuong ChristopheDuong changed the base branch from master to chris/api_catalog February 8, 2021 11:44
@ChristopheDuong ChristopheDuong changed the base branch from chris/api_catalog to chris/namespace-catalog February 8, 2021 13:58
@ChristopheDuong ChristopheDuong force-pushed the chris/namespace-catalog branch from e8de614 to fa26e08 Compare February 8, 2021 14:17
Copy link
Contributor

@michel-tricot michel-tricot left a comment

Choose a reason for hiding this comment

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

I read this PR as a mechanical change. Is there one piece that you don't feel super comfortable with and where I should spend extratime?

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Feb 9, 2021

I read this PR as a mechanical change. Is there one piece that you don't feel super comfortable with and where I should spend extratime?

I would appreciate some pointers/answers from this comment: #1993 (comment) on how to handle the bumpversion...
I think making this backward compatibility is not so easy too if you have any ideas to share

@swyxio swyxio deleted the chris/namespace-destination branch October 12, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Namespace destination output
2 participants