potiuk commented on PR #69740:
URL: https://github.com/apache/airflow/pull/69740#issuecomment-5151596451
Nicely scoped. Defaulting to `False` means existing Dags are untouched,
gating the return on both the flag and `apply_function` avoids surprising
anyone using `apply_function_batch`, and the tests cover the cases that matter
— default-returns-`None`, plus a combined test that filters `None` results and
preserves consume order against a mock consumer that drains progressively
rather than handing everything back at once.
I went looking for the memory and XCom hazard here, since this operator can
run with `read_to_end=True` and no `max_messages`, and you had already covered
it in the docs:
> Returned values use normal task return handling and may be stored in XCom,
so avoid returning large payloads.
One thing before merge: that warning only exists in
`docs/operators/index.rst`, not in the parameter docstring:
```
:param return_apply_function_results: Whether to collect non-None return
values from the per-message
``apply_function`` and return them as a list. This option does not apply
to ``apply_function_batch``.
```
The API reference is where someone looks when deciding whether to switch a
flag on, and that is exactly the moment the size caveat matters. Could you add
a clause there — something like "Results are returned through normal task
return handling and may be stored in XCom, so avoid enabling this for large
result sets"?
While you are in there, it would be worth noting in the same docstring that
`None` results are dropped, so the returned list does not line up positionally
with the consumed messages. It is implied by "non-None return values", but a
caller trying to map results back to offsets could reasonably miss it.
Happy to merge once that's in.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
