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.

Sign up for GitHub

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

Jump to bottom

allow nullable columns to be used as cursor #17131

Merged
merged 7 commits into from
Sep 26, 2022
Merged

allow nullable columns to be used as cursor #17131

merged 7 commits into from
Sep 26, 2022

Conversation

subodh1810
Copy link
Contributor

@subodh1810 subodh1810 self-assigned this Sep 26, 2022
@subodh1810 subodh1810 requested a review from a team as a code owner September 26, 2022 10:42
@github-actions github-actions bot added the area/connectors Connector related issues label Sep 26, 2022
@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-snowflake
  • source-scaffold-java-jdbc
  • source-bigquery
  • source-clickhouse-strict-encrypt
  • source-mysql-strict-encrypt
  • source-clickhouse
  • source-tidb
  • source-mysql
  • source-oracle
  • source-postgres
  • source-cockroachdb-strict-encrypt
  • source-alloydb
  • source-alloydb-strict-encrypt
  • source-oracle-strict-encrypt
  • source-redshift
  • source-mssql-strict-encrypt
  • source-mssql
  • source-db2-strict-encrypt
  • source-cockroachdb
  • source-db2
  • source-postgres-strict-encrypt

@subodh1810
Copy link
Contributor Author

subodh1810 commented Sep 26, 2022 •
edited by github-actions bot
Loading

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3127174644
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3127174644
🐛 https://gradle.com/s/2qfazoowoizqq

Build Failed

Test summary info:

Could not find result summary

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-cockroachdb-strict-encrypt
  • source-postgres-strict-encrypt
  • source-alloydb
  • source-db2
  • source-postgres
  • source-oracle-strict-encrypt
  • source-clickhouse
  • source-db2-strict-encrypt
  • source-mssql-strict-encrypt
  • source-cockroachdb
  • source-snowflake
  • source-mysql-strict-encrypt
  • source-alloydb-strict-encrypt
  • source-tidb
  • source-redshift
  • source-mssql
  • source-scaffold-java-jdbc
  • source-mysql
  • source-bigquery
  • source-clickhouse-strict-encrypt
  • source-oracle

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-db2-strict-encrypt
  • source-mssql
  • source-bigquery
  • source-cockroachdb-strict-encrypt
  • source-redshift
  • source-clickhouse
  • source-postgres
  • source-mysql-strict-encrypt
  • source-mysql
  • source-cockroachdb
  • source-mssql-strict-encrypt
  • source-oracle-strict-encrypt
  • source-scaffold-java-jdbc
  • source-postgres-strict-encrypt
  • source-clickhouse-strict-encrypt
  • source-db2
  • source-alloydb-strict-encrypt
  • source-snowflake
  • source-alloydb
  • source-tidb
  • source-oracle

@subodh1810
Copy link
Contributor Author

subodh1810 commented Sep 26, 2022 •
edited by github-actions bot
Loading

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3127520269
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3127520269
No Python unittests run

Build Passed

Test summary info:

All Passed

edgao
edgao approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

:shipit:, maybe useful followup: should we throw an error if we detect a null in the cursor column?

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Sep 26, 2022
@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-oracle
  • source-db2-strict-encrypt
  • source-postgres
  • source-scaffold-java-jdbc
  • source-snowflake
  • source-postgres-strict-encrypt
  • source-mssql-strict-encrypt
  • source-tidb
  • source-alloydb-strict-encrypt
  • source-db2
  • source-mysql-strict-encrypt
  • source-clickhouse
  • source-cockroachdb-strict-encrypt
  • source-alloydb
  • source-mssql
  • source-oracle-strict-encrypt
  • source-clickhouse-strict-encrypt
  • source-bigquery
  • source-mysql
  • source-cockroachdb
  • source-redshift

@subodh1810
Revert "bump version"
78eb251 
This reverts commit a3cf9af.
@github-actions github-actions bot removed the area/documentation Improvements or additions to documentation label Sep 26, 2022
@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-postgres
  • source-db2
  • source-mysql
  • source-db2-strict-encrypt
  • source-cockroachdb-strict-encrypt
  • source-clickhouse
  • source-mssql-strict-encrypt
  • source-oracle-strict-encrypt
  • source-cockroachdb
  • source-redshift
  • source-alloydb-strict-encrypt
  • source-alloydb
  • source-bigquery
  • source-clickhouse-strict-encrypt
  • source-postgres-strict-encrypt
  • source-tidb
  • source-mssql
  • source-mysql-strict-encrypt
  • source-snowflake
  • source-oracle
  • source-scaffold-java-jdbc

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Sep 26, 2022
@subodh1810
Copy link
Contributor Author

subodh1810 commented Sep 26, 2022 •
edited by github-actions bot
Loading

/publish connector=connectors/source-postgres

🕑 Publishing the following connectors:
connectors/source-postgres
https://github.com/airbytehq/airbyte/actions/runs/3128687094


Connector Did it publish? Were definitions generated?
connectors/source-postgres ✅ ✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-cockroachdb-strict-encrypt
  • source-oracle-strict-encrypt
  • source-redshift
  • source-db2-strict-encrypt
  • source-cockroachdb
  • source-scaffold-java-jdbc
  • source-clickhouse-strict-encrypt
  • source-mysql
  • source-tidb
  • source-db2
  • source-mssql
  • source-mysql-strict-encrypt
  • source-oracle
  • source-bigquery
  • source-postgres-strict-encrypt
  • source-mssql-strict-encrypt
  • source-clickhouse
  • source-snowflake
  • source-alloydb
  • source-alloydb-strict-encrypt
  • source-postgres

ryankfu
ryankfu reviewed Sep 26, 2022
View reviewed changes
Copy link
Contributor

@ryankfu ryankfu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Should there also be the removal of NOT NULL in the setup for tests like in PostgresJdbcSourceAcceptanceTest as well as PostgresSourceTest based on the files changed in the #14356?

@subodh1810
Copy link
Contributor Author

@ryankfu I think its fine. There is a dedicated test to make sure that nullable columns are allowed as cursor

@grishick
Copy link
Contributor

I don't see a test that verifies the behavior, which caused the OC issue: user has a view on top of a table where the cursor field is non-nullable in the source table.

@subodh1810
Copy link
Contributor Author

@grishick the test testDiscoverDifferentGrantAvailability in PostgresSourceTest covers that. We create a bunch of views in that test and I added an assertion to make sure all of em have FULL_REFRESH and INCREMENTAL mode available for selection. This part

@subodh1810 subodh1810 merged commit cb7076b into master Sep 26, 2022
@subodh1810 subodh1810 deleted the allow-nullable-columns-as-cursor branch September 26, 2022 15:50
@edgao
Copy link
Contributor

edgao commented Sep 26, 2022

publishing strict-encrypt in #17147

@bleonard bleonard mentioned this pull request Sep 26, 2022
Closed
pmossman added a commit that referenced this pull request Sep 27, 2022
@pmossman
Revert "allow nullable columns to be used as cursor (#17131)"
507c1b0 
This reverts commit cb7076b.
This was referenced Sep 27, 2022
Closed
Merged
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
@subodh1810 @octavia-squidington-iii @robbinhan
allow nullable columns to be used as cursor (airbytehq#17131)
200f725 
* allow nullable columns to be used as cursor

* add test

* fix test

* bump version

* Revert "bump version"

This reverts commit a3cf9af.

* bump version

* auto-bump connector version [ci skip]

Co-authored-by: Octavia Squidington III <[email protected]>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@subodh1810 @octavia-squidington-iii @jhammarstedt
allow nullable columns to be used as cursor (airbytehq#17131)
8dbf224 
* allow nullable columns to be used as cursor

* add test

* fix test

* bump version

* Revert "bump version"

This reverts commit a3cf9af.

* bump version

* auto-bump connector version [ci skip]

Co-authored-by: Octavia Squidington III <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankfu ryankfu ryankfu left review comments

@edgao edgao edgao approved these changes

@grishick grishick Awaiting requested review from grishick

Assignees

@subodh1810 subodh1810

Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/jdbc connectors/source/mysql connectors/source/mysql-strict-encrypt connectors/source/postgres
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@subodh1810 @grishick @edgao @ryankfu @octavia-squidington-iii