Abacn commented on code in PR #38108:
URL: https://github.com/apache/beam/pull/38108#discussion_r3111723877
##########
sdks/python/apache_beam/typehints/row_type_test.py:
##########
@@ -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)
Review Comment:
register_row calls register_coder so it doesn't diverge that much. My
thought introducing "register_row" was on
- Shorter public facing API (one parameter vs two)
- backward compatibility reason. "register_row" changed the timing when a
schema gets registered. When there is other logical type registry related
workaround, registering schema earlier will break such workaround. This is what
exactly happened in jdbcio test for explicit schema here:
https://github.com/apache/beam/pull/38108/changes#diff-f74abd8bee97790c1ea61a3be880191eda760a12b20df0206ffa67ab7d5a94c7R91
put new behavior in a new API avoided those breakages.
DeprecationWarning can be added after we migrate all register_coder(...,
RowCoder) usage over Beam tests. For now, just added a documentation
recommending `register_row` for RowCoders
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
