TheNeuralBit commented on a change in pull request #11963:
URL: https://github.com/apache/beam/pull/11963#discussion_r459031897



##########
File path: sdks/python/apache_beam/transforms/core.py
##########
@@ -2247,6 +2249,166 @@ def runner_api_requires_keyed_input(self):
     return True
 
 
+def _expr_to_callable(expr, pos):
+  if isinstance(expr, str):
+    return lambda x: getattr(x, expr)
+  elif callable(expr):
+    return expr
+  else:
+    raise TypeError(
+        'Field expression %r at %s must be a callable or a string.' %
+        (expr, pos))
+
+
+class GroupBy(PTransform):
+  """Groups a PCollection by one or more expressions, used to derive the key.
+
+  `GroupBy(expr)` is roughly equivalent to
+
+      beam.Map(lambda v: (expr(v), v)) | beam.GroupByKey()
+
+  but provides several conviniences, e.g.

Review comment:
       ```suggestion
     but provides several conveniences, e.g.
   ```

##########
File path: sdks/python/apache_beam/coders/row_coder.py
##########
@@ -87,7 +87,10 @@ def from_runner_api_parameter(schema, components, 
unused_context):
   @staticmethod
   def from_type_hint(type_hint, registry):
     if isinstance(type_hint, row_type.RowTypeConstraint):
-      schema = named_fields_to_schema(type_hint._fields)
+      try:
+        schema = named_fields_to_schema(type_hint._fields)
+      except ValueError:
+        return typecoders.registry.get_coder(object)

Review comment:
       Ah I see. I'd rather not have this escape hatch if we can avoid it, but 
I can imagine it would be frustrating for users who don't care about using Rows 
xlang if we just refuse to make a RowCoder for them. Perhaps we should make 
`named_fields_to_schema` wrap unknown type hints in a "pythonsdk" logical type 
that uses fast primitives coder. Then we can give a better error message 
if/when a xlang issue arises. If you just add a TODO here I can take that 
action.
   
   When running locally I noticed that this is only needed for your tests 
because we can't infer a type for `sign=x // abs(x) if x else 0` and fallback 
to `Any`. Just curious - is it possible to fix the inference for that function? 
It seems like we should be able to.




----------------------------------------------------------------
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:
[email protected]


Reply via email to