Faster default coder for unknown windows. #33382
Conversation
This will get used in a windowed reshuffle, among other places.
|
R: @shunping |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| @@ -1421,6 +1421,37 @@ def estimate_size(self, value, nested=False): | |||
| return size | |||
|
|
|||
|
|
|||
| class _OrderedUnionCoderImpl(StreamCoderImpl): | |||
| def __init__(self, coder_impl_types, fallback_coder_impl): | |||
| assert len(coder_impl_types) < 128 | |||
There was a problem hiding this comment.
Just out of curiosity, why do we set the upper bound to 128 here? Shouldn't it be 255 given ix==0xFF is reserved for fallback coder in the following code?
There was a problem hiding this comment.
At first I was thinking about avoiding any sign issues, but also this leaves headroom for expanding the protocol in the future.
| def encode_to_stream(self, value, out, nested): | ||
| value_t = type(value) | ||
| for (ix, t) in enumerate(self._types): | ||
| if value_t is t: |
There was a problem hiding this comment.
Do we want to enforce the class strictly or allow matching for subclasses too?
e.g.
if issubclass(value_t, t):
...There was a problem hiding this comment.
I'm comparing exact types here to make the round trip faithful. E.g. a Coder[T] is unlikely to faithfully encode and decode all subclasses of T. This is how fast primitives coder works as well.
|
Thanks for the review. |
This will get used in a windowed reshuffle, among other places.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.