Add Namespace Field. #2704

davinchia · 2021-04-01T15:37:56Z

What

Add namespace field to the Airbyte Stream in preparation to propagate a source defined namespace to the Destination.

This namespace field is then consumed as the destination schema the table is written to.

This only applies to JDBC destinations.

This is Steps 1 - 4 of this tech spec.

Some minor refactoring and commenting as I go.

How

See the above.

Pre-merge Checklist

[x] Run integration test.

Recommended reading order

airbyte_protocol.yaml (main change)
JdbcBufferedConsumerFactory (propagate change to JDBC destinations)
BigQueryDestination (propagate change to BigQuery)
RedshiftCopyDestination and RedshiftCopier (propagate change to Redshift)
TestDestination and various YDestination classes (understand how this change tests)
spec.json (UI changes)
en.json (UI changes)

…aviour.

davinchia · 2021-04-01T15:48:55Z

...ors/destination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/WriteConfig.java

 public class WriteConfig {

  private final String streamName;
-  private final String outputNamespaceName;
+  private final String outputSchemaName;


Since namespace is something that doesn't appear in the destination concept space now, I felt it was clearer to rename this field.

davinchia · 2021-04-01T15:50:10Z

@cgardens want to take a quick sanity check? The destination side implementation is very simple - mostly struggling with testing this on the non-locally available tests. I'm pretty confident; just 2 tests cases and this should be good to go.

…orking for postgres and jdbc destination.

… test working.

davinchia · 2021-04-02T16:52:14Z

...st/src/main/java/io/airbyte/integrations/standardtest/destination/DataArgumentsProvider.java

+ * SOFTWARE.
+ */
+
+package io.airbyte.integrations.standardtest.destination;


Broke this out since the TestDestination class was getting quite large.

davinchia · 2021-04-02T16:52:57Z

...st/src/main/java/io/airbyte/integrations/standardtest/destination/DataArgumentsProvider.java

+ */
+public class DataArgumentsProvider implements ArgumentsProvider {
+
+  public static final CatalogMessageTestConfigPair EXCHANGE_RATE_CONFIG =


also allows us to centralise naming and references to the various test configs here instead of scattered manually typed strings.

davinchia · 2021-04-02T16:56:46Z

...ination-jdbc/src/test/java/io/airbyte/integrations/destination/jdbc/JdbcDestinationTest.java

@@ -1,335 +0,0 @@
-/*


I decided that this test class here was of very little use since:

they are almost an exact duplicate of the JDBC Integration tests in terms of what is being tested.

they serve no real value since the JDBC integration test can be run locally without additional credentials.

the main value this class brings is letting us run tests without building the docker image (the integration tests require doing so). however I feel this benefit is not worth the additional maintenance cost.

Same reasons to removing the Postgres test file.

davinchia · 2021-04-02T17:02:00Z

...ion-test/src/main/java/io/airbyte/integrations/standardtest/destination/TestDestination.java

@@ -127,20 +122,12 @@
   *


There are many changes in this file since I did some refactoring here for readability.

Main changes are adding a namespace field to the retrieveRecords and retrieveNormalizedRecords functions. Adding the getDefaultSchema function to make it easy for destinations to specify the default schema.

I rearrange the test methods so it's

Get

Check(s)

Syncs

Incremental

Normalisation

Namespace

And pushed all the helper functions down. Sadly, can't use nested classes for better organisation here since we are using an abstract class pattern.

the diff on this file is hard to parse. i'm assuming it is just 1. a rearrange, 2 adding the defaultNamespace method, and 3 adding this test: source does not specify a schema. lmk if there's another change in here i should be looking for.

aye apologies. I should have broken the refactoring changes into another commit. Will be better next time.

yes you were looked at all the right bits!

…ination. Running into a strange test error. Checkpointing for now.

davinchia · 2021-04-03T11:51:59Z

/test connector=connectors/destination-csv

🕑 connectors/destination-csv https://github.com/airbytehq/airbyte/actions/runs/714239619
✅ connectors/destination-csv https://github.com/airbytehq/airbyte/actions/runs/714239619

davinchia · 2021-04-03T11:52:10Z

/test connector=connectors/destination-local-json

🕑 connectors/destination-local-json https://github.com/airbytehq/airbyte/actions/runs/714239742
✅ connectors/destination-local-json https://github.com/airbytehq/airbyte/actions/runs/714239742

davinchia · 2021-04-03T12:03:33Z

/test connector=connectors/destination-meilisearch

🕑 connectors/destination-meilisearch https://github.com/airbytehq/airbyte/actions/runs/714253798
✅ connectors/destination-meilisearch https://github.com/airbytehq/airbyte/actions/runs/714253798

davinchia · 2021-04-03T14:12:04Z

/test connector=connectors/destination-redshift

🕑 connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/714464398
❌ connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/714464398
🕑 connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/714464398
✅ connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/714464398

sherifnada

One blocking question

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

sherifnada

Worked out my question - no longer blocking

davinchia · 2021-04-04T02:03:50Z

@sherifnada was my tech spec clear? how did you work it out?

airbyte-integrations/connectors/destination-postgres/src/main/resources/spec.json

davinchia · 2021-04-04T10:11:54Z

airbyte-integrations/connectors/destination-redshift/src/main/resources/spec.json

@@ -11,34 +11,40 @@
    "properties": {
      "host": {
        "description": "Host Endpoint of the Redshift Cluster (must include the cluster-id, region and end with .redshift.amazonaws.com)",
-        "type": "string"


davinchia · 2021-04-04T10:12:37Z

airbyte-integrations/connectors/destination-snowflake/src/main/resources/spec.json

@@ -21,37 +21,44 @@
      "host": {
        "description": "Host domain of the snowflake instance (must include the account, region, cloud environment, and end with snowflakecomputing.com).",
        "examples": ["accountname.us-east-2.aws.snowflakecomputing.com"],
-        "type": "string"


davinchia · 2021-04-04T10:19:13Z

airbyte-webapp/src/locales/en.json

@@ -76,8 +76,8 @@
  "form.edit": "Edit",
  "form.done": "Done",
  "form.examples": "(e.g. {examples})",
-  "form.prefix": "Namespace Prefix",
-  "form.prefix.message": "Where to replicate your data to in your destination",
+  "form.prefix": "Table Prefix",


davinchia · 2021-04-04T10:28:06Z

@cgardens @sherifnada @ChristopheDuong besides publishing the new docker images, the destination side of enabling namespaces is done, so please take a look. Will follow this up with a source side PR to enable this for JDBC sources.

cgardens

Very happy with this. Nice work! I especially like how you added support for testing the namespace stuff to testing. It's concise but I think will give us a lot of security. I also appreciate the screenshots you put in to show how your changes will affect UX.

I think one thing that needs to happen (either in this PR or in a follow up) is adding to the docs for each of these destinations what the expect behavior is around namespaces. Maybe just a new subsection we put in the docs for each destination. e.g. the csv, meiliseach, json ones ignore it. BQ should say namespace corresponds to dataset id, etc. I just want to make sure we don't forget it because it's going to be a nuanced idea for our users to wrap their head around. Also you should think about how how we want to document the "namespace" concept in general in our docs. maybe it gets its own readme page in the architecture section alongside incremental.md, etc?

cgardens · 2021-04-05T22:14:14Z

airbyte-integrations/connectors/destination-bigquery/src/main/resources/spec.json

@@ -16,8 +16,8 @@
      },
      "dataset_id": {
        "type": "string",
-        "description": "The BigQuery dataset id that will house replicated tables.",
-        "title": "Dataset ID"
+        "description": "Default BigQuery Dataset ID tables are replicated to if the source does not specify a schema.",


i think this might be the wrong screenshot? i think this is the one for redshift?

airbyte-integrations/connectors/destination-postgres/src/main/resources/spec.json

...tegration/java/io/airbyte/integrations/destination/redshift/RedshiftCopyIntegrationTest.java

cgardens · 2021-04-05T22:18:25Z

airbyte-webapp/src/locales/en.json

@@ -76,8 +76,8 @@
  "form.edit": "Edit",
  "form.done": "Done",
  "form.examples": "(e.g. {examples})",
-  "form.prefix": "Namespace Prefix",
-  "form.prefix.message": "Where to replicate your data to in your destination",
+  "form.prefix": "Table Prefix",


cgardens · 2021-04-05T22:20:31Z

airbyte-integrations/connectors/destination-bigquery/src/main/resources/spec.json

@@ -16,8 +16,8 @@
      },
      "dataset_id": {
        "type": "string",
-        "description": "The BigQuery dataset id that will house replicated tables.",
-        "title": "Dataset ID"
+        "description": "Default BigQuery Dataset ID tables are replicated to if the source does not specify a schema.",


but reading the actual json, i think it's reasonable.

cgardens · 2021-04-05T22:26:58Z

...ion-test/src/main/java/io/airbyte/integrations/standardtest/destination/TestDestination.java

@@ -127,20 +122,12 @@
   *


the diff on this file is hard to parse. i'm assuming it is just 1. a rearrange, 2 adding the defaultNamespace method, and 3 adding this test: source does not specify a schema. lmk if there's another change in here i should be looking for.

davinchia · 2021-04-06T03:35:50Z

Glad this made sense! Good call on documentation @cgardens. Agree with ya. Will release an actual version + add the necessary documentation in a follow up PR.

I was thinking we should release the new destinations + documentation after the source side changes are done (should be in the next few days) since this isn't useful as is yet. What do you think?

Add namespace field to the Airbyte Stream in preparation to propagate a source defined namespace to the Destination. This namespace field is then consumed as the destination schema the table is written to. This only applies to JDBC destinations. This is Steps 1 - 4 of the namespace tech spec, seen at https://docs.google.com/document/d/1qFk4YqnwxE4MCGeJ9M2scGOYej6JnDy9A0zbICP_zjI/edit. Some minor refactoring and commenting as I go. * Remove unnecessary test classes as they match Integration tests in terms of what is being tested. They have no real value since the corresponding integration test can be run locally without additional credentials. The main value the classes brings is letting us run tests without building the docker image (the integration tests require doing so). however I feel this benefit is not worth the additional maintenance cost. * Centralise DataArgumentProvider into it's own class for easier maintenance and usability.

Davin Chia added 3 commits April 1, 2021 15:06

Add namespace field to AirbyteStream.

8a28bc6

Implement the schema logic. Some slight refactor for readability.

5e069c9

Write tests for new Postgresql and Jdbc Destination on new schema beh…

19e2d60

…aviour.

davinchia changed the title ~~Davinchia/add namespace field~~ Add Namespace Field. Apr 1, 2021

davinchia commented Apr 1, 2021

View reviewed changes

Davin Chia added 4 commits April 2, 2021 14:38

WIP: Introduce namespace into the TestDestination.

c05ccde

WIP: Introduce integration test method into test destination class. W…

bae860f

…orking for postgres and jdbc destination.

Get Postgres Integration and Local test working. Get JDBC Integration…

b08fd1c

… test working.

Remove duplicated tests.

7a108db

davinchia commented Apr 2, 2021

View reviewed changes

Davin Chia added 2 commits April 3, 2021 01:52

Make sure all table names are properly sanitised for RedshiftCopyDest…

4ed3945

…ination. Running into a strange test error. Checkpointing for now.

Get RedshiftTests working.

403b361

Merge remote-tracking branch 'origin' into davinchia/add-namespace-field

7fbe15a

davinchia marked this pull request as ready for review April 3, 2021 12:10

auto-assign bot requested review from cgardens and michel-tricot April 3, 2021 12:10

davinchia requested review from ChristopheDuong and removed request for michel-tricot April 3, 2021 12:27

Is this it?

e74008e

sherifnada suggested changes Apr 3, 2021

View reviewed changes

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml Show resolved Hide resolved

sherifnada reviewed Apr 3, 2021

View reviewed changes

davinchia commented Apr 4, 2021

View reviewed changes

airbyte-integrations/connectors/destination-postgres/src/main/resources/spec.json Show resolved Hide resolved

davinchia commented Apr 4, 2021

View reviewed changes

Change namespace prefix to table prefix.

a345fbe

davinchia commented Apr 4, 2021

View reviewed changes

Merge remote-tracking branch 'origin' into davinchia/add-namespace-field

dca8144

davinchia mentioned this pull request Apr 5, 2021

Populate namespace during Discover Schema. #2736

Closed

2 tasks

cgardens approved these changes Apr 5, 2021

View reviewed changes

sherifnada self-requested a review April 5, 2021 22:38

Davin Chia added 2 commits April 6, 2021 11:16

Use namespace instead of schema in description.

3ffde44

Remove accidentally committed print statements.

1fec464

davinchia merged commit 9f16651 into master Apr 6, 2021

davinchia deleted the davinchia/add-namespace-field branch April 6, 2021 03:50

davinchia mentioned this pull request Apr 6, 2021

Discover Schema sets Namespace field. #2767

Merged

1 task

cgardens mentioned this pull request Apr 6, 2021

Prefix option is not clear in connection page #2674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Namespace Field. #2704

Add Namespace Field. #2704

davinchia commented Apr 1, 2021 •

edited

Loading

davinchia Apr 1, 2021

davinchia commented Apr 1, 2021

davinchia Apr 2, 2021

davinchia Apr 2, 2021

davinchia Apr 2, 2021 •

edited

Loading

davinchia Apr 2, 2021 •

edited

Loading

cgardens Apr 5, 2021

davinchia Apr 6, 2021

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

sherifnada left a comment

sherifnada left a comment

davinchia commented Apr 4, 2021 •

edited

Loading

davinchia Apr 4, 2021

davinchia Apr 4, 2021

davinchia Apr 4, 2021

cgardens Apr 5, 2021

davinchia commented Apr 4, 2021

cgardens left a comment

cgardens Apr 5, 2021

cgardens Apr 5, 2021

cgardens Apr 5, 2021

cgardens Apr 5, 2021

davinchia commented Apr 6, 2021

Add Namespace Field. #2704

Add Namespace Field. #2704

Conversation

davinchia commented Apr 1, 2021 • edited Loading

What

How

Pre-merge Checklist

Recommended reading order

Choose a reason for hiding this comment

davinchia commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

davinchia Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia commented Apr 3, 2021 • edited by github-actions bot Loading

davinchia commented Apr 3, 2021 • edited by github-actions bot Loading

davinchia commented Apr 3, 2021 • edited by github-actions bot Loading

davinchia commented Apr 3, 2021 • edited by github-actions bot Loading

sherifnada left a comment

Choose a reason for hiding this comment

sherifnada left a comment

Choose a reason for hiding this comment

davinchia commented Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia commented Apr 4, 2021

cgardens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia commented Apr 6, 2021

davinchia commented Apr 1, 2021 •

edited

Loading

davinchia Apr 2, 2021 •

edited

Loading

davinchia Apr 2, 2021 •

edited

Loading

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

davinchia commented Apr 3, 2021 •

edited by github-actions bot

Loading

davinchia commented Apr 4, 2021 •

edited

Loading