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

refactor: only add streams with field transforms to updatedStreams #50

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

teallarson
Copy link
Contributor

@teallarson teallarson commented Nov 9, 2023

closes airbytehq/airbyte#16092

BEFORE:
Previously, we would add all streams with changes to uodatedStreams. However, within each stream, the only information we report back is (a) Field type changes or (b) Field addition/removal (name changes would be a removal/addition).

This means that some types of changes, for instance a source changing its source-defined primary key or cursor, would add a stream to updatedStreams that would have an empty array for its transforms.

AFTER:
These types of changes are not considered breaking. We don't need the information for the Platform's purposes, and we have not had a feature request to surface this kind of information in the diff modal.

We now only add streams with a relevant fieldTransform to updatedStreams.

// getStreamDiff only checks for differences in the stream's field name or field type
// but there are a number of reasons the streams might be different (such as a source-defined
// primary key or cursor changing). These should not be expressed as "stream updates".
UpdateStreamTransform streamTransform =getStreamDiff(streamOld, streamNew, stream);
Copy link
Contributor

Choose a reason for hiding this comment

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

linting will address the spacing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Ran formatting + added a test for good measure.

@flutra-osmani
Copy link
Contributor

flutra-osmani commented Nov 9, 2023

solves ticket

@teallarson teallarson changed the title Only add streams with field transforms to updatedStreams refactor: only add streams with field transforms to updatedStreams Nov 9, 2023
@teallarson teallarson marked this pull request as ready for review November 9, 2023 20:11
@teallarson teallarson marked this pull request as draft November 13, 2023 19:14
Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

This is a bug fix in helper conversion code.

@teallarson teallarson marked this pull request as ready for review November 14, 2023 16:24
@teallarson
Copy link
Contributor Author

teallarson commented Nov 14, 2023

This is a bug fix in helper conversion code.

@davinchia Are you saying the PR title should be "fix:" or just noting that?

@teallarson teallarson merged commit c501898 into main Nov 16, 2023
6 checks passed
@teallarson teallarson deleted the teal/empty-transforms branch November 16, 2023 15:14
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.

catalogDiff object gives incomplete information about primary key changes
3 participants