-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-3761]Fix Python 3 cmp usage #4774
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
Changes from all commits
16beed9
d190bc2
979e540
14b74b1
d1419e9
aeb66a6
2880bb2
0bdca44
cd98dae
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 |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| """ | ||
|
|
||
| from abc import ABCMeta | ||
| from abc import abstractmethod | ||
| from functools import total_ordering | ||
|
|
||
| from apache_beam import coders | ||
| from apache_beam import core | ||
|
|
@@ -46,44 +46,65 @@ class Event(object): | |
|
|
||
| __metaclass__ = ABCMeta | ||
|
|
||
| def __cmp__(self, other): | ||
| if type(self) is not type(other): | ||
| return cmp(type(self), type(other)) | ||
| return self._typed_cmp(other) | ||
|
|
||
| @abstractmethod | ||
| def _typed_cmp(self, other): | ||
| raise NotImplementedError | ||
|
|
||
|
|
||
| @total_ordering | ||
| class ElementEvent(Event): | ||
| """Element-producing test stream event.""" | ||
|
|
||
| def __init__(self, timestamped_values): | ||
| self.timestamped_values = timestamped_values | ||
|
|
||
| def _typed_cmp(self, other): | ||
| return cmp(self.timestamped_values, other.timestamped_values) | ||
| def __eq__(self, other): | ||
| return (type(self) is type(other) and | ||
| self.timestamped_values == other.timestamped_values) | ||
|
|
||
| def __ne__(self, other): | ||
|
Member
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. Do you need to implement ne, is not this the default implementation?
Member
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. @holdenk what do you think?
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. In Python3 we are fine but in Python 2.7, ne isn't inherited from object. If you see the docs for total_ordering. See the notes in https://bugs.python.org/issue25732
Member
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. Thank you. |
||
| return not self.__eq__(other) | ||
|
|
||
| def __lt__(self, other): | ||
| if type(self) is not type(other): | ||
| return type(self) < type(other) | ||
| return self.timestamped_values < other.timestamped_values | ||
|
|
||
|
|
||
| @total_ordering | ||
| class WatermarkEvent(Event): | ||
| """Watermark-advancing test stream event.""" | ||
|
|
||
| def __init__(self, new_watermark): | ||
| self.new_watermark = timestamp.Timestamp.of(new_watermark) | ||
|
|
||
| def _typed_cmp(self, other): | ||
| return cmp(self.new_watermark, other.new_watermark) | ||
| def __eq__(self, other): | ||
| return (type(self) is type(other) and | ||
| self.new_watermark == other.new_watermark) | ||
|
|
||
| def __ne__(self, other): | ||
| return not self.__eq__(other) | ||
|
|
||
| def __lt__(self, other): | ||
| if type(self) is not type(other): | ||
| return type(self) < type(other) | ||
| return self.new_watermark < other.new_watermark | ||
|
|
||
|
|
||
| @total_ordering | ||
| class ProcessingTimeEvent(Event): | ||
| """Processing time-advancing test stream event.""" | ||
|
|
||
| def __init__(self, advance_by): | ||
| self.advance_by = timestamp.Duration.of(advance_by) | ||
|
|
||
| def _typed_cmp(self, other): | ||
| return cmp(self.advance_by, other.advance_by) | ||
| def __eq__(self, other): | ||
| return (type(self) is type(other) and | ||
| self.advance_by == other.advance_by) | ||
|
|
||
| def __ne__(self, other): | ||
| return not self.__eq__(other) | ||
|
|
||
| def __lt__(self, other): | ||
| if type(self) is not type(other): | ||
| return type(self) < type(other) | ||
| return self.advance_by < other.advance_by | ||
|
|
||
|
|
||
| class TestStream(PTransform): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -193,7 +193,7 @@ def __cmp__(left, right): # pylint: disable=no-self-argument | |
| on unequal values (always returning 1). | ||
| """ | ||
| if type(left) is not type(right): | ||
| return cmp(type(left), type(right)) | ||
| return 1 | ||
|
Member
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. This is a change in behaviour even though it matches the comment above. What does this affect?
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. All of the tests pass. If you were to sort a windowed value with something which was not a windowed value the ordering could change.
Member
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. Sounds good. cc: @robertwb since this pertains to the todo below. |
||
|
|
||
| # TODO(robertwb): Avoid the type checks? | ||
| # Returns False (0) if equal, and True (1) if not. | ||
|
|
||
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.
@charlesccychen
Where do we compare these events?
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.
Ping?