github-actions[bot] commented on code in PR #41334:
URL: https://github.com/apache/beam/pull/41334#discussion_r4573027459
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -485,14 +506,39 @@ def _getattr(o, name):
return Const(BoundMethod(func, o))
elif isinstance(o, row_type.RowTypeConstraint):
return o.get_type_for(name)
Review Comment:
**🟠High**: `RowTypeConstraint.get_type_for(name)` returns the raw field
annotation without calling `typehints.normalize(...)`. For schema-generated
`NamedTuple`s with nullable nested rows (e.g.,
`ReadFromParquet(as_rows=True)`), `schemas.named_tuple_from_schema` annotates
the struct field as `typing.Optional[RowTypeConstraint(fields=[('col',
typing.Optional[T]), ...])]`. When resolving `r.struct_col.col`, `_getattr`
recurses into this `RowTypeConstraint` branch and receives a raw
`typing.Optional[T]` instead of a `UnionConstraint`. Consequently,
`_flatten_union_members` does not flatten it, and `_union_of` constructs a
nested `Union[Union[NoneType, T], NoneType]`, breaking
`typehints.get_concrete_type_from_nullable` and deeper attribute access
(`r.a.b.c`). Normalizing the result here ensures `RowTypeConstraint` returns a
Beam typehint consistent with the `NamedTuple` and `dataclass` branches.
```suggestion
return typehints.normalize(o.get_type_for(name))
```
##########
sdks/python/apache_beam/typehints/opcodes.py:
##########
@@ -485,14 +506,39 @@ def _getattr(o, name):
return Const(BoundMethod(func, o))
elif isinstance(o, row_type.RowTypeConstraint):
return o.get_type_for(name)
- elif inspect.isclass(o) and dataclasses.is_dataclass(o):
- field = o.__dataclass_fields__.get(name)
+ elif inspect.isclass(o):
+ if match_is_named_tuple(o):
+ field_type = o.__annotations__.get(name)
+ if field_type is None:
+ return Any
+ try:
+ return typehints.normalize(field_type)
+ except Exception: # Inference should never fail.
+ return Any
+ field = (o.__dataclass_fields__.get(name)
+ if dataclasses.is_dataclass(o) else None)
if field is not None:
return resolve_dataclass_field_type(field.type)
return Any
-
else:
- return Any
+ try:
+ normalized = typehints.normalize(o)
+ except Exception:
+ return Any
+ if not isinstance(normalized, typehints.UnionHint.UnionConstraint):
+ return Any
+ has_none = type(None) in normalized.union_types
+ resolved = [
+ _getattr(member, name)
+ for member in normalized.union_types
+ if member is not type(None)
+ ]
+ if not resolved or any(result is Any for result in resolved):
Review Comment:
**🟠High**: If a method is accessed on an `Optional` or `Union` type (for
example `r.address._asdict()` on `Optional[Address]`, or any method call in
Python 3.12+ where `LOAD_METHOD` was replaced by `LOAD_ATTR`),
`_getattr(member, name)` returns `Const(BoundMethod(func, member))`. Because
`Const` is not `Any`, execution proceeds to `_union_of(members)` and
`Union[...]` raises `TypeError: All parameters to a Union hint must be a
non-sequence, a type, or a TypeConstraint. Const[...] is an instance of Const`,
crashing `infer_return_type(..., debug=True)` or aborting inference for the
entire enclosing function. We should bail out to `Any` when any resolved member
is a `Const`.
```suggestion
if not resolved or any(result is Any or isinstance(result, Const) for
result in resolved):
```
--
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]
