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

Add primary key tests to TestDestination #2776

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

ChristopheDuong
Copy link
Contributor

@ChristopheDuong ChristopheDuong commented Apr 6, 2021

What

Closes #2749
Relates to #2683
Closes #2684

How

Describe the solution
For the test case, I also used the scenario where we would want to deduplicate using as the cursor the column _airbyte_emitted_at column as described in #2683

This is because exchange rate API could have the "date" column as the primary key but we can have multiple rows for the same day and there isn't another date column in the data. By doing so, the cursor field is now optional for normalization.

This is also handling a column with type float as described in #2684 as the primary key.

(normalization should now be able to handle the empty/airbyte generated column use case as cursor and is a matter of handling that option in the UI too)

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. airbyte-integrations/bases/standard-destination-test/src/main/java/io/airbyte/integrations/standardtest/destination/TestDestination.java
  2. airbyte-integrations/bases/base-normalization/normalization/transform_catalog/stream_processor.py
  3. the rest

@auto-assign auto-assign bot requested review from cgardens and jrhizor April 6, 2021 19:37
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.

Nice! Great test.

s.withSyncMode(SyncMode.INCREMENTAL);
s.withDestinationSyncMode(DestinationSyncMode.APPEND_DEDUP);
s.withCursorField(List.of("_airbyte_emitted_at"));
s.withPrimaryKey(List.of(List.of("date")));
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 a good test. would it make sense to test a compound primary key as well? exchange rates is actually a good candidate for this where you can imagine the primary key being [currency, date].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but Exchange rates connector doesn't actually include a "base" currency column (as selected in the connector's spec.json), I'll add one in the catalog/message json files then.

However moving on this idea, I added a currency value column (actual conversion rate float number) as part of the primary key. It may not make much business sense to do that but it is a failing use case in #2684

@@ -502,7 +497,11 @@ def generate_scd_type_2_model(self, from_table: str, column_names: Dict[str, Tup

def get_cursor_field(self, column_names: Dict[str, Tuple[str, str]]) -> str:
if self.cursor_field and len(self.cursor_field) == 1:
return column_names[self.cursor_field[0]][0]
if not is_airbyte_column(self.cursor_field[0]):
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

column_names is a map of all original columns declared in the properties/JSON schema for the current table.

But, here we want to allow generated airbyte columns as cursor fields or primary key too (those may not be listed in the original JSON schema) so I add that special case with this if statement

ChristopheDuong and others added 2 commits April 7, 2021 10:31
Co-authored-by: Charles <giardina.charles@gmail.com>
@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Apr 7, 2021

/test connector=connectors/destination-bigquery

🕑 connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/725941392
✅ connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/725941392

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Apr 7, 2021

/test connector=connectors/destination-csv

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

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Apr 7, 2021

/test connector=connectors/destination-local-json

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

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Apr 7, 2021

/test connector=connectors/destination-meilisearch

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

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Apr 7, 2021

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/725941946
✅ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/725941946

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Apr 7, 2021

/test connector=connectors/destination-redshift

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

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Apr 7, 2021

/test connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/725942080
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/725942080

@ChristopheDuong ChristopheDuong changed the title Add primary tests to TestDestination Add primary key tests to TestDestination Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment