You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/03 17:26:01 UTC
[GitHub] [beam] kennknowles opened a new issue, #18350: Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps
kennknowles opened a new issue, #18350:
URL: https://github.com/apache/beam/issues/18350
It seems like these APIs are predominantly duplicative of each other and, that it's hard to find one of them if you knew about the other.
https://github.com/apache/beam/blob/348d335883b14a9b143b65e4b3c62dc79f62d77e/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/WithTimestamps.java#L134
https://github.com/apache/beam/blob/348d335883b14a9b143b65e4b3c62dc79f62d77e/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Create.java#L560
What would make the most sense to me is if TimestampedValues were implemented in terms of both Values and WithTimestamps. I'm still learning about Beam though \-- would this approach cause some kind of performance problem?
Imported from Jira [BEAM-1811](https://issues.apache.org/jira/browse/BEAM-1811). Original Jira may contain additional context.
Reported by: wtanaka.
--
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-unsubscribe@beam.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps [beam]
Posted by "xujiantop-crypto (via GitHub)" <gi...@apache.org>.
xujiantop-crypto commented on issue #18350:
URL: https://github.com/apache/beam/issues/18350#issuecomment-5599664928
.take-issue
--
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-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps [beam]
Posted by "Gaurav598 (via GitHub)" <gi...@apache.org>.
Gaurav598 commented on issue #18350:
URL: https://github.com/apache/beam/issues/18350#issuecomment-5494812158
.free-issue
--
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-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps [beam]
Posted by "Gaurav598 (via GitHub)" <gi...@apache.org>.
Gaurav598 commented on issue #18350:
URL: https://github.com/apache/beam/issues/18350#issuecomment-5495277845
I investigated the current implementation of this issue and wanted to leave a few findings here that may be useful to whoever picks it up next.
The duplication between `WithTimestamps.AddTimestampsDoFn` and `Create.TimestampedValues.ConvertTimestamps` is still present: both ultimately use `outputWithTimestamp(...)`.
However, a direct `Create.Values + WithTimestamps` implementation does not appear to be a straightforward replacement. The timestamp is part of each `TimestampedValue<T>` and is not necessarily derivable from `T`, so duplicate/equal values can have different timestamps.
I also found that `Create.TimestampedValues` was previously related to `Create.Values` through inheritance, but that was intentionally removed because it caused runner matching issues. Reintroducing that relationship would therefore be risky.
The current `TimestampedValues` implementation also deliberately:
- infers the coder from the underlying `T` values,
- uses `TimestampedValueCoder` for the intermediate collection,
- unwraps the timestamped values through `ConvertTimestamps`,
- and explicitly restores the final `Coder<T>`.
Because of this, a conservative internal abstraction for the common timestamp-output behavior seems safer than restructuring `TimestampedValues` around `Values + WithTimestamps`. This would allow the existing execution path, coder behavior, and transform structure to remain unchanged.
The relevant tests also include renderer and Dataflow runner expectations around the current `Create.TimestampedValues`/`ConvertTimestamps` structure, so those are worth considering before changing the pipeline graph.
Hopefully these findings save some investigation time for the next contributor.
--
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-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps [beam]
Posted by "Gaurav598 (via GitHub)" <gi...@apache.org>.
Gaurav598 commented on issue #18350:
URL: https://github.com/apache/beam/issues/18350#issuecomment-5471309357
Hi @kennknowles @damccorm , I investigated the current implementation of `WithTimestamps` and `Create.TimestampedValues` to understand the safest way to address this issue.
The duplication is still present: both `AddTimestampsDoFn` and `ConvertTimestamps` ultimately emit a value using `outputWithTimestamp(...)`. However, the surrounding behavior differs in important ways.
In particular, implementing `TimestampedValues` directly as `Create.Values + WithTimestamps` does not appear to be a safe refactor. The timestamp is not necessarily a function of `T`, so equal/duplicate values can legitimately have different timestamps. Preserving that would require keeping `TimestampedValue<T>` as an intermediate representation and then unwrapping it, which would add another transform/`ParDo` and change the current pipeline graph.
I also found that `TimestampedValues` was previously related to `Create.Values` through inheritance, but that was intentionally changed because it caused runner matching issues. So I don't think reintroducing that relationship would be appropriate.
My current understanding is that the safest approach is a small internal/package-private abstraction for the common timestamp-output behavior, while preserving the existing `Create.TimestampedValues` execution path, coder handling, transform structure, and `WithTimestamps` skew semantics.
I would be happy to implement this approach if that direction looks reasonable.
.take-issue
--
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-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles closed issue #18350: Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps
URL: https://github.com/apache/beam/issues/18350
--
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-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Extract common class for WithTimestamps.AddTimestampsDoFn and Create.TimestampedValues.ConvertTimestamps [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on issue #18350:
URL: https://github.com/apache/beam/issues/18350#issuecomment-5605416039
TBH I don't think this is a good idea. This creates an inheritance-based coupling when just having this trivial code in two places is more flexible and keeps control flow simpler.
--
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-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org