-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Use registered type for Row #38108
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
base: master
Are you sure you want to change the base?
Use registered type for Row #38108
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| { | ||
| "comment": "Modify this file in a trivial way to cause this test suite to run", | ||
| "modification": 1 | ||
| "modification": 16 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| { | ||
| "comment": "Modify this file in a trivial way to cause this test suite to run", | ||
| "modification": 1 | ||
| "modification": 17 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| { | ||
| "comment": "Modify this file in a trivial way to cause this test suite to run", | ||
| "modification": 1 | ||
| "modification": 3 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,17 +33,18 @@ class RowTypeTest(unittest.TestCase): | |
| @staticmethod | ||
| def _check_key_type_and_count(x) -> int: | ||
| key_type = type(x[0]) | ||
| if not row_type._user_type_is_generated(key_type): | ||
| raise RuntimeError("Expect type after GBK to be generated user type") | ||
| if row_type._user_type_is_generated(key_type): | ||
| raise RuntimeError("Type after GBK not preserved, get generated type") | ||
| if not hasattr(key_type, row_type._BEAM_SCHEMA_ID): | ||
| raise RuntimeError("Type after GBK missing Beam schema ID") | ||
|
|
||
| return len(x[1]) | ||
|
|
||
| def test_group_by_key_namedtuple(self): | ||
| MyNamedTuple = typing.NamedTuple( | ||
| "MyNamedTuple", [("id", int), ("name", str)]) | ||
|
|
||
| beam.coders.typecoders.registry.register_coder( | ||
| MyNamedTuple, beam.coders.RowCoder) | ||
| beam.coders.typecoders.registry.register_row(MyNamedTuple) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure the divegence between Can we either
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. register_row calls register_coder so it doesn't diverge that much. My thought introducing "register_row" was on
DeprecationWarning can be added after we migrate all register_coder(..., RowCoder) usage over Beam tests. For now, just added a documentation recommending |
||
|
|
||
| def generate(num: int): | ||
| for i in range(100): | ||
|
|
@@ -67,8 +68,7 @@ class MyDataClass: | |
| id: int | ||
| name: str | ||
|
|
||
| beam.coders.typecoders.registry.register_coder( | ||
| MyDataClass, beam.coders.RowCoder) | ||
| beam.coders.typecoders.registry.register_row(MyDataClass) | ||
|
|
||
| def generate(num: int): | ||
| for i in range(100): | ||
|
|
@@ -120,10 +120,8 @@ class DataClassInt: | |
| class DataClassStr(DataClassInt): | ||
| name: str | ||
|
|
||
| beam.coders.typecoders.registry.register_coder( | ||
| DataClassInt, beam.coders.RowCoder) | ||
| beam.coders.typecoders.registry.register_coder( | ||
| DataClassStr, beam.coders.RowCoder) | ||
| beam.coders.typecoders.registry.register_row(DataClassInt) | ||
| beam.coders.typecoders.registry.register_row(DataClassStr) | ||
|
|
||
| def generate(num: int): | ||
| for i in range(10): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,7 +26,7 @@ | |||||||||||||||||
| class SchemaTypeRegistry(object): | ||||||||||||||||||
| def __init__(self): | ||||||||||||||||||
| self.by_id = {} | ||||||||||||||||||
| self.by_typing = {} | ||||||||||||||||||
| self.by_typing = {} # currently not used | ||||||||||||||||||
|
|
||||||||||||||||||
| def generate_new_id(self): | ||||||||||||||||||
| for _ in range(100): | ||||||||||||||||||
|
|
@@ -43,6 +43,15 @@ def add(self, typing, schema): | |||||||||||||||||
| if schema.id: | ||||||||||||||||||
| self.by_id[schema.id] = (typing, schema) | ||||||||||||||||||
|
|
||||||||||||||||||
| def load_registered_typings(self, by_id): | ||||||||||||||||||
| for id, typing in by_id.items(): | ||||||||||||||||||
| if id not in self.by_id: | ||||||||||||||||||
| self.by_id[id] = (typing, None) | ||||||||||||||||||
|
Comment on lines
+46
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable name
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No actual conflict happening. schema object has an "id" field (schema.id). |
||||||||||||||||||
|
|
||||||||||||||||||
| def get_registered_typings(self): | ||||||||||||||||||
| # Used by save_main_session, as pb2.schema isn't picklable | ||||||||||||||||||
| return {k: v[0] for k, v in self.by_id.items()} | ||||||||||||||||||
|
|
||||||||||||||||||
| def get_typing_by_id(self, unique_id): | ||||||||||||||||||
| if not unique_id: | ||||||||||||||||||
| return None | ||||||||||||||||||
|
|
||||||||||||||||||
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 test was passing before, but this line is needed after #38078