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
CDK: Add schema inferrer class #20941
CDK: Add schema inferrer class #20941
Changes from 9 commits
2a5a789
1f2b520
59f8010
1d75e1e
a67e59d
c89cc10
62c78af
11f8ef9
0550aa2
8a582f4
0e610aa
2a633b9
47954d3
edc6b11
87b8b93
a4e7687
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.
The original tech spec outlined the
accumulate
andget_inferred_schemas
functions, but for context what's the use case for a single stream or how are we using thisget_stream_schema()
functionAlso why do we no longer need the
reset()
?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.
fwiw when writing the spec i didn't account for this object needing to infer schema on multiple streams (that iface assumed a single stream). I think this iface makes more sense.
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.
Yeah, when integrating with the API I noticed that for that use case we only need a specific stream and this API seemed nicer. The full
get_inferred_schemas
will still be nice for the planned integration with theread
command / implementing a separate command for schema inferenceThere 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 dropped the
reset
because in the places where we plan to use this class it's much easier to just lose the reference to the inferrer instance and have the runtime clean it up, then create a new instance for the next usage. Happy to change that, but it doesn't seem heavy enough to explicitly keep an instance or a pool of them around.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.
would recommend considering the use of
pytest.mark.fixture
here to parametrize tests