damccorm commented on code in PR #35725:
URL: https://github.com/apache/beam/pull/35725#discussion_r2305209237
##########
sdks/python/apache_beam/coders/coders.py:
##########
@@ -180,7 +180,8 @@ def is_deterministic(self):
"""
return False
- def as_deterministic_coder(self, step_label, error_message=None):
+ def as_deterministic_coder(
+ self, step_label, error_message=None, update_compatibility_version=None):
Review Comment:
> I think it will break coders impl's that dont include
update_compatibility_version in the method signature, is that what you meant?
>
> And the fix would be define a new function in base class like
as_deterministic_coder_v2 that by defers to as_deterministic_coder by default,
unless implemented by the specific coder?
Yeah, this is what I'm suggesting.
> We could also catch the error with a message to update the method
signature, but not fail in this case.
This seems good as well.
##########
sdks/python/apache_beam/coders/coders.py:
##########
@@ -180,7 +180,8 @@ def is_deterministic(self):
"""
return False
- def as_deterministic_coder(self, step_label, error_message=None):
+ def as_deterministic_coder(
+ self, step_label, error_message=None, update_compatibility_version=None):
Review Comment:
> I think it will break coders impl's that dont include
update_compatibility_version in the method signature, is that what you meant?
>
> And the fix would be define a new function in base class like
as_deterministic_coder_v2 that by defers to as_deterministic_coder by default,
unless implemented by the specific coder?
Yeah, this is what I'm suggesting.
> We could also catch the error with a message to update the method
signature, but not fail in this case.
This seems good as well, I am fine with either path
--
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: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
