AnandInguva commented on code in PR #29849:
URL: https://github.com/apache/beam/pull/29849#discussion_r1439574855
##########
sdks/python/apache_beam/typehints/schemas.py:
##########
@@ -343,14 +343,8 @@ def typing_to_runner_api(self, type_: type) ->
schema_pb2.FieldType:
argument = None
if logical_type.argument_type() is not None:
argument_type = self.typing_to_runner_api(logical_type.argument_type())
- try:
- argument = self.value_to_runner_api(
- argument_type, logical_type.argument())
- except ValueError:
Review Comment:
are there cases where this value error could still happen?
##########
sdks/python/apache_beam/typehints/schemas.py:
##########
@@ -529,7 +541,7 @@ def typing_from_runner_api(
return Any
else:
return LogicalType.from_runner_api(
- fieldtype_proto.logical_type).language_type()
+ fieldtype_proto.logical_type)._language_type()
Review Comment:
why do we need to change to `_language_type()`? we can reuse the
`language_type` method itself right?
##########
sdks/python/apache_beam/typehints/schemas_test.py:
##########
@@ -511,10 +580,8 @@ def assert_namedtuple_equivalent(self, actual, expected):
#for attr in dir(expected):
# self.assertEqual(getattr(actual, attr), getattr(expected, attr))
- @parameterized.expand([
- (fieldtype_proto, )
- for fieldtype_proto in get_test_beam_fieldtype_protos()
- ])
+ @parameterized.expand([(fieldtype_proto, ) for fieldtype_proto in
+ get_test_beam_fieldtype_protos_with_logical_types()])
Review Comment:
This replaces the `get_test_beam_fieldtype_protos`. Can you add both
`get_test_beam_fieldtype_protos_with_logical_types` and
`get_test_beam_fieldtype_protos` to the tests?
--
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]
