Remove Dv2 Null checks#31149
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
will do a fuller review later, but can you write tests for bq+snowflake to test the migration e2e? a la https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/destination-snowflake/src/test-integration/java/io/airbyte/integrations/destination/snowflake/typing_deduping/AbstractSnowflakeTypingDedupingTest.java#L160 (i.e. run a sync on a specific version, then run a sync on the dev version) I think that's more powerful than doing it in the sqlgenerator tests - one of the rare instances where I actually prefer dockerized tests |
|
Edit: figured it out! |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Edward Gao (edgao)
left a comment
There was a problem hiding this comment.
🚀
Had some comments around code cleanliness, lmk if you want me to just push those myself + get this merged
This comment was marked as outdated.
This comment was marked as outdated.
|
| Step | Result |
|---|---|
| Build connector tar | ✅ |
| Build destination-bigquery docker image for platform(s) linux/x86_64 | ✅ |
| Java Connector Unit Tests | ✅ |
| Java Connector Integration Tests | ✅ |
| Validate metadata for destination-bigquery | ✅ |
| Connector version semver check | ✅ |
| Connector version increment check | ✅ |
| QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=destination-bigquery test
|
| Step | Result |
|---|---|
| Build connector tar | ✅ |
| Build destination-snowflake docker image for platform(s) linux/x86_64 | ✅ |
| Java Connector Unit Tests | ✅ |
| Java Connector Integration Tests | ✅ |
| Validate metadata for destination-snowflake | ✅ |
| Connector version semver check | ✅ |
| Connector version increment check | ✅ |
| QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=destination-snowflake testCo-authored-by: evantahler <evantahler@users.noreply.github.com> Co-authored-by: Edward Gao <edward.gao@airbyte.io>
This PR removes null-PK checks from the destinations. The sources really shouldn't be emitting records with null PKs, but if they are, we should accept them. Yes, this will possibly lead to strange deduplication behavior... but that's been the case for months now. This makes syncs more likely to succeeded. Detecting records which have null primary keys should be the job of the platform (#31186).
This is also a performance improvement as we remove a raw table scan looking for null-PK records.
We will do another soft-reset to remove the NOT NULL constraints on PK columns in the final tables which may have been added while #30779 was active (rolled back by #31082)