[BEAM-10854] Fix PeriodicImpulse for default values #14446
Conversation
| @@ -66,6 +66,7 @@ def __init__(self, seconds=0, micros=0): | |||
| raise TypeError( | |||
| 'Cannot interpret %s %s as micros.' % (micros, type(micros))) | |||
| self.micros = int(seconds * 1000000) + int(micros) | |||
| self.seconds = seconds + micros / 1000000 | |||
There was a problem hiding this comment.
I do not believe we want to add another field to keep track of the same information. A few options:
- In periodicsequence.py read micros and convert to seconds there.
- hava an attribute on Timestamp here to return seconds from the internally stored micros.
(same for the Duration change below)
There was a problem hiding this comment.
Good point, I was considering using the @property and a _get_seconds(). What do you think?
There was a problem hiding this comment.
I have coded both options and I think the first approach looks better and more compact. I will push the version that changes to seconds directly in periodicsequence
| @@ -37,6 +37,11 @@ | |||
| class ImpulseSeqGenRestrictionProvider(core.RestrictionProvider): | |||
| def initial_restriction(self, element): | |||
| start, end, interval = element | |||
| if isinstance(start, Timestamp): | |||
There was a problem hiding this comment.
Should they be always Timestamps to avoid isinstance checks?
There was a problem hiding this comment.
I guess that would also work, I just didn't want to take the option of using other types, since some users may already be using them.
There was a problem hiding this comment.
Reviewing the code, our current tests use time.time() which return a float, I think we should not change it to only allow Timestamps. What do you think?
There was a problem hiding this comment.
Sounds good. Let's keep it as is.
| @@ -81,6 +86,9 @@ def process( | |||
| ''' | |||
| start, _, interval = element | |||
|
|
|||
| if isinstance(start, Timestamp): | |||
There was a problem hiding this comment.
Not directly related to your change. But this code has some other issues.
current_output_timestamp is not a Timestamp and needs to be converted later.
mixed use of Timestamp and timestamp.Timestamp - we can pick either.
|
It LGTM. I will leave it to @pabloem to merge, in case there will be issues after the merge I won't be available. |
Fixing issue from BEAM-10854 and BEAM-11620,
PeriodicImpulsewould fail with default values.Some notes:
periodicsequence.pycould go directly inPeriodicImpulse(after line 162). But this would only fixPeriodicImpulseand would fail if other users decide to useTimestampinstead ofFloatfor the times.periodicsequence_test.py, adding this line would work to check if the default values would work both forstartandend. This would avoid new errors in case the transform is modified. Those lines could be removed if neededThank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.