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

Discover Schema sets Namespace field. #2767

Merged
merged 8 commits into from
Apr 7, 2021

Conversation

davinchia
Copy link
Contributor

@davinchia davinchia commented Apr 6, 2021

What

This PR is step 5 of this tech spec.

Follow up PR to #2704.

The first of (at least) 2 PRs to implement this on the source side. I made some headway before deciding to break the changes into one PR implementing this for discover schema job, and another PR implementing this for read. The combined PR would have been too big otherwise.

I had to refactor the MoreResources class along the way. Left inline comments explaining why.

I've scoped the changes pretty tightly by commit, so feel free to review by commit (especially for the MoreResources change).

How

  • Expose schema in the TableInfo class in AbstractJdbcSource. Use this to set an Airbyte Stream's namespace field in the Catalog. Update tests.
  • Add the namespace field to the Airbyte Stream struct in the Airbyte Api. Update tests.

Pre-merge Checklist

  • Run integration tests

Recommended reading order

  1. AbstractJdbcSource for main change and AbstractJdbcSourceStandardTest for matching testing changes (Discover changes).
  2. config.yaml, CatalogConverter and AcceptanceTest (understand API changes).
  3. ManyResources (to understand the method that was failing for me)

The various test classes propagate patterns in the above classes, so not terribly interesting.

Davin Chia added 3 commits April 6, 2021 17:24
…le depending on where is class is located. E.g. this does not work when running tests using the Intellij runner. Write to a tmp file instead.
@davinchia
Copy link
Contributor Author

davinchia commented Apr 6, 2021

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/722254282
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/722254282

@davinchia
Copy link
Contributor Author

davinchia commented Apr 6, 2021

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/722254700
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/722254700

@davinchia
Copy link
Contributor Author

davinchia commented Apr 6, 2021

/test connector=connectors/source-mssql

🕑 connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/722302053
✅ connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/722302053

@davinchia
Copy link
Contributor Author

davinchia commented Apr 6, 2021

/test connector=connectors/source-jdbc

🕑 connectors/source-jdbc https://github.com/airbytehq/airbyte/actions/runs/722302315
✅ connectors/source-jdbc https://github.com/airbytehq/airbyte/actions/runs/722302315

@davinchia
Copy link
Contributor Author

davinchia commented Apr 6, 2021

/test connector=connectors/source-oracle

🕑 connectors/source-oracle https://github.com/airbytehq/airbyte/actions/runs/722303250
❌ connectors/source-oracle https://github.com/airbytehq/airbyte/actions/runs/722303250
🕑 connectors/source-oracle https://github.com/airbytehq/airbyte/actions/runs/722303250
❌ connectors/source-oracle https://github.com/airbytehq/airbyte/actions/runs/722303250

@davinchia davinchia marked this pull request as ready for review April 6, 2021 13:02
@davinchia davinchia requested review from cgardens and jrhizor and removed request for michel-tricot and ChristopheDuong April 6, 2021 13:03
@davinchia
Copy link
Contributor Author

davinchia commented Apr 6, 2021

Aside form the e2e_docker test and the oracle test (both of which are passing on my local system but failing on here, not sure why, will look tmrw), this is done.

I am going to hold off on any connector version release until my next PR to remove the schema name from the stream name.

@@ -49,10 +49,18 @@ public static AirbyteStream createAirbyteStream(String streamName, Field... fiel
return createAirbyteStream(streamName, Arrays.asList(fields));
}

public static AirbyteStream createAirbyteStream(String streamName, String schemaName, Field... fields) {
Copy link
Contributor Author

@davinchia davinchia Apr 6, 2021

Choose a reason for hiding this comment

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

Some method duplication here now; I will remove them in my next PR as part of removing schema names from stream names.

Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

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

looks good. please make sure it works with mysql which doesn't have schemas. i think we have generally treated the db name for mysql as the equivalent of schema or namespace.

@davinchia davinchia merged commit 58062fa into master Apr 7, 2021
@davinchia davinchia deleted the davinchia/namespace-discover-schema branch April 7, 2021 03:53
@davinchia davinchia mentioned this pull request Apr 8, 2021
14 tasks
davinchia added a commit that referenced this pull request Apr 12, 2021
Last step (besides documentation) of namespace changes. This is a follow up to #2767 .

After this change, the following JDBC sources will change their behaviour to the behaviour described in the above document.

Namely, instead of streamName = schema.tableName, this will become streamName = tableName and namespace = schema. This means that, when replicating from these sources, data will be replicated into a form matching the source. e.g. public.users (postgres source) -> public.users (postgres destination) instead of current behaviour of public.public_users. Since MySQL does not have schemas, the MySQL source uses the database as it's namespace.

I cleaned up some bits of the CatalogHelpers. This affected the destinations, so I'm also running the destination tests.
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.

2 participants