-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
10de6c6
to
28e46b6
Compare
@@ -1,7 +1,7 @@ | |||
CREATE | |||
TABLE | |||
id_and_name( | |||
id INTEGER, | |||
id INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
53a5c0c
to
d79d785
Compare
There was a problem hiding this 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
There was a problem hiding this 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!
Thanks for the review Ed and Ryan! |
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.