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
[BEAM-10475] Add typehints for ShardedKeyCoder #13474
Conversation
from apache_beam.utils.sharded_key import ShardedKey | ||
|
||
|
||
class ShardedKeyTypeConstraint(typehints.TypeConstraint): |
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.
To verify: this type constraint is for Beam internal use only and users are not expected specify it?
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.
Users or dev may use it for specifying the output type of a transform to provide type hints.
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 you do want to allow something like a ShardedKey
type hint annotation in code, add it here:
Any = AnyTypeConstraint() |
(doesn't have to be in this PR)
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 see. Thanks for pointing it out. I will change that in a follow-up PR.
2097994
to
6cc9e10
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.
LGTM, thanks for the thorough tests!
LMK if I can merge this or if you have more changes to make
def test_type_check_not_sharded_key(self): | ||
constraint = ShardedKeyTypeConstraint(int) | ||
obj = 5 | ||
with self.assertRaises(TypeError) as e: |
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.
Note: self.assertRaisesRegex
can also check exception messages.
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 to know!
Thanks for the review! This is good to go. |
Implement
to_type_hint
andfrom_type_hint
forShardedKeyCoder
and add aTypeConstraint
forShardedKey
so during the runner API translation the corresponding coder can be added.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.