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

Fix build by marking id columns as primary keys in test files #15774

Merged
merged 1 commit into from
Aug 19, 2022

Conversation

pmossman
Copy link
Contributor

In #14356 we changed how supported sync modes are determined: a source stream must have at least one field that can be used as a cursor in order for the stream to support incremental syncing.

The Postgres sources that we use in acceptance test weren't declaring id columns as primary keys, which meant they were nullable and thus could not be used as cursor columns.

This broke some of our acceptance tests that tested the behavior of incremental syncs. This PR modifies the seed sql we use in these tests to declare the id column as a primary key, so that incremental can be supported.

@pmossman pmossman force-pushed the parker/fix-incremental-acceptance-tests branch from 10de6c6 to 28e46b6 Compare August 18, 2022 23:50
@pmossman pmossman temporarily deployed to more-secrets August 18, 2022 23:53 Inactive
@pmossman pmossman temporarily deployed to more-secrets August 19, 2022 00:36 Inactive
@pmossman pmossman temporarily deployed to more-secrets August 19, 2022 01:59 Inactive
@pmossman pmossman temporarily deployed to more-secrets August 19, 2022 02:04 Inactive
@@ -1,7 +1,7 @@
CREATE
TABLE
id_and_name(
id INTEGER,
id INTEGER PRIMARY KEY,
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be INTEGER NOT NULL? (this is a mild preference just to minimize the change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, that should work without requiring any changes to the test logic

@pmossman pmossman force-pushed the parker/fix-incremental-acceptance-tests branch from 53a5c0c to d79d785 Compare August 19, 2022 02:57
@pmossman pmossman temporarily deployed to more-secrets August 19, 2022 03:00 Inactive
Copy link
Contributor

@ryankfu ryankfu left a comment

Choose a reason for hiding this comment

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

This makes sense to me since it roughly matches the tests in PostgresJdbcSourceAcceptanceTest.java although in those tests the primary key is not set as INTEGER NOT NULL so would like to have the build pass as a sanity check before approval

Copy link
Contributor

@ryankfu ryankfu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for getting the build back to green!

@pmossman pmossman merged commit a3a6501 into master Aug 19, 2022
@pmossman pmossman deleted the parker/fix-incremental-acceptance-tests branch August 19, 2022 03:59
@pmossman
Copy link
Contributor Author

Thanks for the review Ed and Ryan!

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.

3 participants