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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
