Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
update api for per stream #13835
update api for per stream #13835
Changes from 3 commits
97f01b7
bce906e
48211b7
113db67
d00b004
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Random thought - should the CatalogDiff contain other changes to streams besides just additions, removals, and field schema changes?
For example, should we be communicating changes to any of the following fields for a stream in the CatalogDiff?
My guess is that we aren't putting these changes in the diff because the diff is just meant to communicate schema changes, but I wanted to double check.
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.
great point! in the future, yes. i don't think we need it at this state. that's why i structure this as a list with each item having a type instead of a list per type. i think the number of types will grow.
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.
That makes sense! Yeah definitely not worth adding now if we don't need it at the moment, and this seems like an extensible approach that will allow us to add more stream transformation types in the future as we need them 👍
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.
From the discussion I had with Tim, this should be a schema and be an object with no constraint (i.e.
additionalProperties = true
).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.
@benmoriceau aha! thanks for linking the comment in the google doc. i hadn't seen that. long term, i'm not sure that passing the schema around is a great pattern, but since that is the pattern already for how the FE handles the catalog, it makes sense, i'll switch to that.
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.
If this is being changed to a schema, maybe we should also rename these properties from
type
->schema
, e.g.update_field_schema
,fieldSchema
,FieldSchemaUpdate
, etc.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.
agreed.
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.
Just checking - do we have an issue for implementing this endpoint specifically? Looking at the whimsical board, I only see issues for implementing the CatalogDiff behavior
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.
I was bundling it in with this issue #13648.