github-actions[bot] commented on code in PR #41201: URL: https://github.com/apache/beam/pull/41201#discussion_r4535484409
########## sdks/python/apache_beam/typehints/arrow_type_compatibility.py: ########## @@ -295,13 +304,71 @@ def _arrow_type_from_beam_fieldtype( return output_arrow_type +def _from_arrow_value_fn(typ) -> Optional[Callable[[Any], Any]]: + """Returns a function that turns a value from ``pa.Scalar.as_py`` into + ``typ``, or None if the value needs no conversion. + + ``as_py`` returns structs as dicts, so nested rows are built from those + dicts, including rows in optional fields and in lists. + """ + if isinstance(typ, RowTypeConstraint): + return _row_from_dict_fn(typ.user_type, typ._fields) + if match_is_named_tuple(typ): + return _row_from_dict_fn( + typ, [(name, typ.__annotations__[name]) for name in typ._fields]) + if match_dataclass_for_row(typ): + return _row_from_dict_fn( + typ, [(field.name, field.type) for field in dataclasses.fields(typ)]) + + origin = get_origin(typ) + args = get_args(typ) + if origin is Union: Review Comment: **🟠High**: `_from_arrow_value_fn` relies on `typing.get_origin(typ)` and `origin is Union`, which misses two cases: 1. **Normalized Beam `TypeConstraint`s:** `BatchConverter.from_typehints` normalizes `element_type` via `typehints.normalize(element_type)` before instantiating `PyarrowArrayBatchConverter` (and Beam typehints like `beam.typehints.Optional[...]` or `beam.typehints.List[...]` can also appear in row fields). For `UnionConstraint` and `SequenceTypeConstraint` subclasses (`ListConstraint`, `ABCSequenceTypeConstraint`, etc.), `typing.get_origin(typ)` returns `None`, so `BatchConverter.from_typehints(element_type=Optional[Address], batch_type=pa.Array)` or `BatchConverter.from_typehints(element_type=List[Address], batch_type=pa.Array)` will have `self._convert = None` and yield raw `dict`s instead of `Address` rows. 2. **PEP 604 union syntax (`Address | None`):** On Python 3.10+, `get_origin(Address | None)` is `types.UnionType` rather than `typing.Union`. Consider converting non-row `TypeConstraint`s via `convert_to_python_type(typ)` first (as `schemas.py` does in `typing_to_runner_api`) and checking `origin in (Union, types.UnionType)`. -- 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]
