BEAM-7995: python PGBKCVOperation using wrong timestamp #9364
Conversation
python PGBKCVOperation is using closed interval end which is not acceptable for window calculation
| @@ -886,7 +886,7 @@ def output_key(self, wkey, accumulator): | |||
| if windows is 0: | |||
| self.output(_globally_windowed_value.with_value((key, value))) | |||
| else: | |||
| self.output(WindowedValue((key, value), windows[0].end, windows)) | |||
| self.output(WindowedValue((key, value), windows[0].max_timestamp(), windows)) | |||
There was a problem hiding this comment.
Would not this make the timestamp for the element incorrect?
There was a problem hiding this comment.
Why is it any more incorrect than "end"? max_timestamp = end - 0.001
There was a problem hiding this comment.
You are right. I read this as just 'max_timestamp()', did not realize it is the max timestamp for the window. Your change looks correct.
Referencing your email (https://lists.apache.org/thread.html/ad42c55ed0212cb18b2b29bfc3dddfb47b8cb9f3358583775d0da37b@%3Cdev.beam.apache.org%3E), this needs to be a shared definition between Python and Java. @lukecwik could you confirm that an element in a window should have a timestamp < window.end or could it be <= window.end ?
There was a problem hiding this comment.
This change certainly fixes a bug (though I agree the docs could and should be cleaned up).
We should file a JIRA that we're not supporting more general timestamp combining fns here.
|
Run Python PreCommit |
|
PGBKCV = PartialGroupByKeyCombineValues
represents the mappers side lifted combine
https://s.apache.org/beam-runner-api-combine-model
…On Fri, Aug 23, 2019 at 4:23 PM Kyle Weaver ***@***.***> wrote:
This is only tangential to this PR, but while we're here, can someone tell
me what "PGBKCV" stands for? 😅 I'd like to see this documented.
https://issues.apache.org/jira/browse/BEAM-8087
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9364?email_source=notifications&email_token=ACM4V3ABPBSYAVK2C5B74YTQGBWPDA5CNFSM4IMM4NOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BRSRA#issuecomment-524491076>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACM4V3EDA54H42UT7GN323LQGBWPDANCNFSM4IMM4NOA>
.
|
|
Run Python PreCommit |
|
Run Portable_Python PreCommit |
|
@aaltay thank you so much for the lint fix. Do you happen to know whether anyone is looking into the python pre-commit failure on master branch? It seems to have been broken for a while. I plan to send an email to ask about this but just in case that you have some info. |
|
Test was failing with another lint error. I fixed it in github's UI and did not run linter on it. You can check the test logs to see there are other issues. |
|
Run Portable_Python PreCommit |
|
@aaltay ah, thank you so much. Since now all checks have passed. Can you help merge this PR? |
BEAM-7995: python PGBKCVOperation using wrong timestamp (apache#9364) python PGBKCVOperation is using closed interval end which is not acceptable for window calculation
python PGBKCVOperation is using closed interval end which is not acceptable for window calculation. Make a change to use window.max_timestamp() which uses open interval end for the window
Thank 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.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.