lostluck commented on PR #24307: URL: https://github.com/apache/beam/pull/24307#issuecomment-1324376805
This is a very good start! > A note about testing: I was unable to write a batch pipeline that resulted in a window mapping operation. > > [Here](https://github.com/camphillips22/beam/compare/cam/mapwindows...camphillips22:beam:mapwindows-manual-tests?expand=1) is a link to the tests I wrote which did not result in a map_windows operation. It also contains the changes I made to the streaming wordcap example to make it happen on dataflow. I have not written a pipeline yet that works on Flink and uses map windows, but I wanted to go ahead get this change out for review. I looked into the Dataflow's graph analyzer and it doesn't use map windows in batch, so that's not amazing for batch testing. We should at least manually validate in streaming mode. I can do this if you like. We should still have unit tests for the MapWindows Node in the exec package. We should test to the spec (with the KV<nonce, Window> inputs and outputs), per the proto. eg.That we preserve the "nonce", and similar. https://github.com/apache/beam/blob/master/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto#L285 I've got a few more specific comments in a moment or two. -- 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]
