[GitHub] [beam] robertwb commented on a change in pull request #11452: [BEAM-9692] Move apply_Read to PTransformOverride

2020-05-01 Thread GitBox


robertwb commented on a change in pull request #11452:
URL: https://github.com/apache/beam/pull/11452#discussion_r418694513



##
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##
@@ -117,13 +117,15 @@ class DataflowRunner(PipelineRunner):
   # TODO: Remove the apache_beam.pipeline dependency in 
CreatePTransformOverride
   from apache_beam.runners.dataflow.ptransform_overrides import 
CombineValuesPTransformOverride
   from apache_beam.runners.dataflow.ptransform_overrides import 
CreatePTransformOverride
-  from apache_beam.runners.dataflow.ptransform_overrides import 
ReadPTransformOverride
   from apache_beam.runners.dataflow.ptransform_overrides import 
JrhReadPTransformOverride
+  from apache_beam.runners.dataflow.ptransform_overrides import 
ReadPTransformOverride
+  from apache_beam.runners.dataflow.ptransform_overrides import 
NativeReadPTransformOverride
 
-  # Thesse overrides should be applied before the proto representation of the
+  # These overrides should be applied before the proto representation of the
   # graph is created.
   _PTRANSFORM_OVERRIDES = [
-  CombineValuesPTransformOverride()
+  CombineValuesPTransformOverride(),

Review comment:
   Correct, this change does not change the existing behavior, but the 
concern is that the existing behavior might be sub-optimal. 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [beam] robertwb commented on a change in pull request #11452: [BEAM-9692] Move apply_Read to PTransformOverride

2020-04-29 Thread GitBox


robertwb commented on a change in pull request #11452:
URL: https://github.com/apache/beam/pull/11452#discussion_r417471423



##
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##
@@ -117,13 +117,15 @@ class DataflowRunner(PipelineRunner):
   # TODO: Remove the apache_beam.pipeline dependency in 
CreatePTransformOverride
   from apache_beam.runners.dataflow.ptransform_overrides import 
CombineValuesPTransformOverride
   from apache_beam.runners.dataflow.ptransform_overrides import 
CreatePTransformOverride
-  from apache_beam.runners.dataflow.ptransform_overrides import 
ReadPTransformOverride
   from apache_beam.runners.dataflow.ptransform_overrides import 
JrhReadPTransformOverride
+  from apache_beam.runners.dataflow.ptransform_overrides import 
ReadPTransformOverride
+  from apache_beam.runners.dataflow.ptransform_overrides import 
NativeReadPTransformOverride
 
-  # Thesse overrides should be applied before the proto representation of the
+  # These overrides should be applied before the proto representation of the
   # graph is created.
   _PTRANSFORM_OVERRIDES = [
-  CombineValuesPTransformOverride()
+  CombineValuesPTransformOverride(),

Review comment:
   @ananvay we should look at how these interact with auto-opt-in at the 
service.

##
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner_test.py
##
@@ -594,6 +594,121 @@ def test_combine_values_translation(self):
 self.assertIn(
 u'CombineValues', set(step[u'kind'] for step in job_dict[u'steps']))
 
+  def test_read_create_translation(self):

Review comment:
   These three feel very redundant; can a common helper method be used here?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org